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

Add --skip-magic-trailing-comma #1824

Merged
merged 10 commits into from
Jan 18, 2021

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 15, 2020

Lukasz indicated he'd accept such a PR in #1288 (comment)

This is a pretty straightforward combination of things from before and after #1612 and #826

Feedback I'm looking for:

  1. What the best way to get the MAGIC_TRAILING_COMMA option to the places it's needed is.
    Obviously the global variable proof of concept is unmergeable, but I'm not sure what the right solution is. Passing it around seems kind of untenable because Leaf.append is used in a million places / should_split_body_explode has a daunting reverse call graph too (You could make store an attribute on Leaf to make things a little easier, but that's pretty ugly as well). Maybe a context manager to set a global current Mode?
  2. How best to structure tests for this
    Black doesn't have many options, so there isn't a clear pattern for how to test these. I went with reusing expression.py, but I could do something more akin to numeric_literals_skip_underscores. Let me know!

@cooperlees cooperlees requested a review from ambv November 15, 2020 07:41
@cooperlees cooperlees added C: configuration CLI and configuration T: enhancement New feature or request F: trailing comma Full of magic labels Nov 15, 2020
@ambv
Copy link
Collaborator

ambv commented Nov 15, 2020

You're right that using global variables won't be acceptable for this. Hiding the global variable by running a context manager is not fundamentally changing this either.

Having an attribute on Leaf is what I would look at.

@hauntsaninja
Copy link
Collaborator Author

Thanks! You'll notice from the diff that I meant to say Line, not Leaf — hope it's still acceptable :-) I also updated blackd and the relevant docs that I could find.

@graingert
Copy link
Contributor

@hauntsaninja thanks for following up on this, I'd completely missed the concept ack on the other issue

@hauntsaninja
Copy link
Collaborator Author

@ambv if you have time to take a look, much obliged!

@hauntsaninja
Copy link
Collaborator Author

Just bumping this!

@@ -438,6 +438,9 @@ into one item per line.
How do you make it stop? Just delete that trailing comma and _Black_ will collapse your
collection into one line if it fits.

If you must, you can recover the behaviour of early versions of Black with the option
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might need to be consistent here at some point:

(py38) mbpt-jelle:black jelle$ git grep behavior | wc -l
       7
(py38) mbpt-jelle:black jelle$ git grep behaviour | wc -l
       7

@JelleZijlstra JelleZijlstra merged commit 692c0f5 into psf:master Jan 18, 2021
@hauntsaninja hauntsaninja deleted the skiptrailingcomma branch January 18, 2021 01:10
@uriva
Copy link

uriva commented Jan 30, 2021

any idea when will this be released?

@ichard26
Copy link
Collaborator

@uriva unfortunately I can't give you a timeframe, but I do hope it will happen soon! Apologies for the lack of any clear messaging.

@uriva
Copy link

uriva commented Feb 24, 2021

@ichard26 what is this blocked on? or is releasing itself a difficult process? (not demanding anything, just curious)

@ichard26
Copy link
Collaborator

@uriva I honestly don't know 100% why a new release is blocked, but the general lack of time and specifically the lack of availability for the ones holding the power to create a release is probably why. Maybe @cooperlees can explain in more detail.

@uriva
Copy link

uriva commented Mar 24, 2021

who has this power?

@graingert
Copy link
Contributor

@uriva you can see the list here https://pypi.org/project/black/#data

@indigoviolet
Copy link

tangential comment: I found this option --skip-trailing-magic-comma in the github documentation, but then didn't find it in the most recent release and it was mildly confusing. It might be useful to either have per-feature metadata like [Added in 20.0b2dev] etc or a develop branch.

@ichard26
Copy link
Collaborator

@indigoviolet you're not alone! I find it confusing too. I am planning to just move the documentation to ReadTheDocs where the stable version will be provided by default to avoid this mess in the first place (it's part of my effort to revamp the docs). Thanks for the feedback though!

@hauntsaninja
Copy link
Collaborator Author

Opened #2064 to add this to CHANGES.md

Separately, is there anything blocking a release that I could help with?

@adamjstewart
Copy link

I tried adding:

skip_magic_trailing_comma = true

to my pyproject.toml but it doesn't seem that this setting can be configured from a file. Could support for this be added?

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Oct 26, 2021

Adding skip_magic_trailing_comma = true in the tool.black section in pyproject.toml definitely works for me. I'd recommend asking in the discord for help or opening an issue if you can't get it to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: configuration CLI and configuration F: trailing comma Full of magic T: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants