-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Proposal: Shades of Black #3087
Comments
This comment was marked as off-topic.
This comment was marked as off-topic.
Just some feedback from a regular user: would you consider turning on all shades by default? One additional point, more related to implementation, is that it would be good to create something like |
This seems like a solid idea to me, provided that we indeed take into consideration the fact that we still want to reduce formatting options. But as I understand it, these wouldn't even be options per-se, just opt-outs. Let me try to formulate some arguments wrt. the amount of shades we want. This would mean having shades as Jelle described them vs. something like "absolutely AST safe" and "all the way", with something in between for removing useless f-strings and other clearly safe but AST-changing things but not e.g. docstring mods. Having more would mean that users get to use more features of Black if a specific transformation is undesirable. But it would also mean more configuration, i.e. more things to fiddle with and argue over, and less consistency in code bases using Black. Having less would conversely mean that configuration is simple, but people might not want to go all the way if some single transformation is undesirable. Applying less shades religiously would mean we'd have to think about removing toggles for the comma and string quotes, which seems like something we don't want either. Or maybe they should be considered special and left as individual toggles. I'm not sure. If possible, I'd like to avoid Black becoming like a collection of tools. We've considered rejecting import sorting many times because @DanielNoord Wrt. turning everything on by default: I appreciate the fact that we want to be as Black as possible, but we have provided pretty useful safety guarantees. Throwing that away seems like a bad thing to do, since after applying Black, it could be a huge pain to revert everything, not to mention that the AST safety check also catches bugs. Of course, version control is used for a reason, but it could be a painful experience if precautions are not taken. Personally, I'd prefer less shades, although I'm not sure what would be the best way to go about it or what the reasonable categories would be. But I'm also sympathetic to Jelle's remarks about avoiding a fragmented ecosystem. Thoughts? |
I'd caution strongly against adding shades:
I really value Black as a single centralized source of uncontroversial zero-config autoformatting. If the Black maintainers are interested in providing semantic changes as well I'd be delighted, but prefer to see that built and stabilized in a downstream package before considering whether it should be upstreamed. |
I agree with @Zac-HD. An opt-out is still an option. There's a huge benefit to the lack of options which leads to a consistency across many projects. I think the choice between Black's options/shades would lead to more bikeshedding and therefore less usage, than the choice between Black and some other tool. See also Prettier's option philosophy:
|
I'm in the "do nothing" camp. Duplicating the capabilities of Note that I'm not opposed to unsafe things like |
I also agree with @Zac-HD However, I do see value in permitting a general opt-out for AST changing translations. Perhaps:
Personally, I think that import sorting (and other functionality at that sort of level) is outside of Black's remit. Removing useless f-strings and However, being able to turn individual translations off and on takes us further down the slippery slope (that started with the magic comma) that is at odds with the idea that Black is opinionated so you don't have to be. |
The biggest point of controversy is currently probably docstrings, if we don't go further into import sorting and similar stuff (which again, I would like us to avoid). So Maybe it's a stupid argument to claim that we should not change AST "where it's conceivable that the meaning of the program changes", the stupidity being the assumption that we can determine what is "conceivable". But I think that could be the criterion to decide if a change is in scope for Black. With import sorting the concern is related to execution order, with docstrings it's external tools reading them. But surely removing unnecessary f-strings is safe 😬 Until someone will crawl out of the woodwork to claim that their tool depends on the presence of unnecessary string prefixes to denote something in the source code. Aaanyways, perhaps we could manage three categories: |
Hi @felix-hilden, I think that the controversial thing here might be how we manage controversial changes! I am not against Black adopting controversial changes but I think that, if adopted, they should be unconditional. If they aren't appropriate to be made unconditional, they are not appropriate for Black (IM<HO). In other words, a controversial change should be debated regarding whether (Black should remove useless f-strings, for example) and how (e.g. should With For AST-changing translations, some will be low enough risk to go straight to All IM<HO, of course! 🤣 |
I mostly agree! Indeed discussing and managing the changes will be the ongoing battle. And "is this appropriate to apply unconditionally" seems like a good litmus test. I'd like to think our testing and review process is good enough to let experimental features in At this point it would be really nice to hear from other maintainers as well! |
I'm also a "do nothing" camp member here too and totally want to echo all the points @Zac-HD made. I like our current |
I'm personally holding out against using black until docstring manipulations and other AST changing features are disabled by default, as per the rationale made clear by @bryevdv in bokeh/bokeh#9636 (comment). Features like isort and docstring should opt-in, black should guarantee that the AST is never changed without specific configuration directing it to do so. |
Sorry to keep bringing this up, but I feel this issue needs to be addressed sooner rather than later. There are a slew of currently open issues that would have a clear path to resolution (perhaps not in the manner that people were expecting) should black give a guarantee the AST is never changed by default:
I acknowledge that some might consider that it's too late, and others will lament any change in direction. But I feel it's better to address core issues such as this as early as possible. Of significance to the core philosophy behind black, the Model T Ford became popular in a large part because it was reliable:
Auto-formatters that change the AST will always be inherently unreliable. |
Perhaps safe to say that having Shades(TM) in their full glory is rejected. We most probably won't revert all unsafe changes: the new string processing is great! Docstring modifications are clearly the biggest pain point beyond wanting strict AST-safety. I sympathise with Jelle on wanting Black to be the one-stop formatter, but as we've rejected import sorting, docstring formatting seems similarly like a different problem that we could leave to other tools. So we could:
Personally I'd be okay with both, although I doubt we'll be able to match potential formatters for different styles of docstrings and all that complexity. So maybe I'm skewing a bit towards the latter option. |
Strong vote for the latter from me! Configuration is terrible, pointing to other tools is great. Let the agglomeration happen in downstream formatters, which can wrap black - and die off and be replaced without causing ecosystem-wide issues. |
After discussing with the team internally, we've decided to:
So this is the hard "do nothing" option. I'm sorry to disappoint, but the advantages of us formatting docstrings outweigh the rare cases where making such modifications is problematic. And consolidating formatting choices is preferable to strict separation of concerns (using another tool). Now all that's left is to make the processing as good as possible 👍 Thank you for the discussion, to me it's been a fruitful one since now we've clarified what we want to do exactly. |
@felix-hilden Hi, could you up elaborate on what the "advantages of us formatting docstrings" are? |
We often get proposals for formatting changes that could be useful, but that change the AST or are otherwise unsafe. Examples include:
black
changes ASTs #2150)not (x in y)
(not (x in y) -> x not in y; not (x is y) -> x is not y #212)Many of these proposals make sense as an extension of Black's mission to create a single, consistent style for Python code, but users have legitimate concerns about the safety of such changes.
Relatedly, there are some tools that wrap Black together with other formatters:
However, these tools have not reached Black's level of popularity. Creating this logic in Black itself would make it more discoverable for users.
I observed that several of the existing formatting options can be described as "either leave it alone, or let Black make its decision":
--skip-string-normalization
,--skip-magic-trailing-comma
. That's a useful principle to follow: formatting options shouldn't be about using one style or another, only about whether to handle some aspect of formatting at all.Proposal
So here's the proposal: There is only one Black code style, but you get a choice how far you commit to it. We'd introduce a system of shades, aspects of formatting that can be individually turned on or off depending on what is safe on your codebase. Initial shades could include:
del
parentheses, but we could make a few other things go into this group (Replace f-strings with regular strings #3081, not (x in y) -> x not in y; not (x is y) -> x is not y #212). These would be things that change the AST, but in minor ways that we're confident don't change the meaning of the program.In the future, we could add additional shades for import sorting, annotation formatting, and other automatic improvements to Python code.
We could add shortcut options for enabling all shades, like
black --all-the-way
for enabling everything, andblack --super-safe
for disabling all AST-unsafe options for the paranoid among us. (Names could be improved.)These new shades would be orthogonal to
--preview
style and the stability policy: any new shades would be enabled only in--preview
mode until the new year starts.Alternatives
The text was updated successfully, but these errors were encountered: