Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-implement magic trailing comma handling #1612

Merged
merged 8 commits into from Aug 21, 2020
Merged

Conversation

ambv
Copy link
Collaborator

@ambv ambv commented Aug 20, 2020

Re-implement magic trailing comma handling:

  • when a trailing comma is specified in any bracket pair, that signals to Black that this bracket pair needs to be always exploded, e.g. presented as "one item per line";

  • this causes some changes to previously formatted code that erroneously left trailing commas embedded into single-line expressions;

  • internally, Black needs to be able to identify trailing commas that it put itself compared to pre-existing trailing commas. We do this by using/abusing lib2to3's was_checked attribute. It's True for internally generated trailing commas and False for pre-existing ones (in fact, for all pre-existing leaves and nodes).

Fixes #1288

Thank you, Sentry, for sponsoring this effort.

@ambv
Copy link
Collaborator Author

ambv commented Aug 20, 2020

Primer will obviously fail due to expected formatting changes. We'll be dealing with the other failures here, looks like they're infrastructure failures.

@ambv
Copy link
Collaborator Author

ambv commented Aug 20, 2020

LOL, such a rookie mistake. Will fix momentarily.

@zsol
Copy link
Collaborator

zsol commented Aug 20, 2020

This looks great! Much clearer logic than before. I spot checked some of the primer changes and they look good too

- when a trailing comma is specified in any bracket pair, that signals to Black
  that this bracket pair needs to be always exploded, e.g. presented as "one
  item per line";

- this causes some changes to previously formatted code that erroneously left
  trailing commas embedded into single-line expressions;

- internally, Black needs to be able to identify trailing commas that it put
  itself compared to pre-existing trailing commas. We do this by using/abusing
  lib2to3's `was_checked` attribute.  It's True for internally generated
  trailing commas and False for pre-existing ones (in fact, for all
  pre-existing leaves and nodes).

Fixes #1288
@ichard26
Copy link
Collaborator

ichard26 commented Aug 21, 2020

I'm curious on why the auto generated documentation files from the README are included with your "fix dealing with generated files in docs" commit. Is it a mistake or am I missing something?

@ambv
Copy link
Collaborator Author

ambv commented Aug 21, 2020

Those files were always committed. Look at the history of black/docs/authors.md. A few of the new ones were missing so I added those, too.

@ambv ambv merged commit 205f3b6 into master Aug 21, 2020
@ambv ambv deleted the better_trailing_comma branch August 21, 2020 14:45
@zyv
Copy link
Contributor

zyv commented Aug 21, 2020

❤️ ❤️ ❤️

@ichard26
Copy link
Collaborator

ichard26 commented Aug 21, 2020

Ah that explains it, thanks! When I look back at the history, those files like black/docs/authors.md were simlinks instead of regular markdown documents. IIRC, those simlinks were causing trouble with GitHub Actions since they were pointing at non existing files. My only concern is these files may become outdated as documentation changes roll in. Oh well, it's probably on me to remind people on updating the autogenerated files as a triager ... :)

@WhyNotHugo
Copy link
Contributor

Oh well, it's probably on me to remind people on updating the autogenerated files as a triager

Might make sense to add a local pre-commit hook to rebuild them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finish "magic trailing comma" handling
5 participants