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

Proposal: Shades of Black #3087

Closed
JelleZijlstra opened this issue May 26, 2022 · 17 comments
Closed

Proposal: Shades of Black #3087

JelleZijlstra opened this issue May 26, 2022 · 17 comments
Labels
R: rejected This will not be worked on T: enhancement New feature or request T: style What do we want Blackened code to look like?

Comments

@JelleZijlstra
Copy link
Collaborator

We often get proposals for formatting changes that could be useful, but that change the AST or are otherwise unsafe. Examples include:

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:

  • String normalization: on by default; if off, string quotes are left alone.
  • Docstrings: on by default; if off, docstrings are not changed at all.
  • Misc AST unsafe: on by default; if off (and other AST-changing options are off), we guarantee full AST safety. I believe this would currently only affect 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.
  • Magic trailing comma: off by default; if on, we ignore pre-existing magic trailing commas.

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, and black --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

  • Do nothing. The proposal adds significant complexity to Black, and we've billed ourselves as a simple, no-configuration tool. I have a lot of sympathy for this position. But if we don't do it, people will build other tools for additional formatting, creating a more fragmented ecosystem that's harder for users to navigate. Black has a chance to provide a much simpler solution to users.
@JelleZijlstra JelleZijlstra added the T: enhancement New feature or request label May 26, 2022
@ichard26 ichard26 pinned this issue May 26, 2022
@ichard26 ichard26 added T: style What do we want Blackened code to look like? S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) labels May 26, 2022
@graingert

This comment was marked as off-topic.

@DanielNoord
Copy link
Contributor

Just some feedback from a regular user: would you consider turning on all shades by default?
I think one of the main benefits of black is that after initial discussion about adoption has passed there is no additional discussion about which checks to turn on or off. I consider that to be something very valuable. By turning some shades on and some shades off you are already signalling to users that shades are opinionated and open to discussion, which runs counter to that.

One additional point, more related to implementation, is that it would be good to create something like isort profiles or eslint's shareable configurations. Over on pylint after years of adding configuration options we are now jealous of such easy ways to share configuration presets.
Being able to do black --preset=project-with-style-I-like --shades-off=this-one-format-I-really-dislike would be quite nice! This is not a suggestion to also add those with this issue, but keeping such potential future changes in mind while writing the implementation might help!

@felix-hilden
Copy link
Collaborator

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 isort already exists. I don't know if the same can be said about docstrings, but that could be a good case for another specialised tool. We do our thing, and to step into another tool's way while compromising our own ethos of minimal configuration and safe formatting seems off.

@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?

@Zac-HD
Copy link
Contributor

Zac-HD commented Jun 4, 2022

I'd caution strongly against adding shades:

  • The minimal-configuration aspect of black is very valuable for user experience across projects.
  • Semantic changes are much more contentious than formatting alone. not x in y -> x not in y would be fine, but I don't think it's feasible to build consensus on how to sort imports - and IMO that means it should be left to other tools.
  • This seems like a poor fit for the once-a-year stability policy.
  • I'm also concerned about the maintainence burden on Black maintainers; supporting shades is likely to mean that the project can never really approach 'finished'.

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.

@hugovk
Copy link
Contributor

hugovk commented Jun 5, 2022

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:

Prettier has a few options because of history. But we won’t add more of them.
...
By far the biggest reason for adopting Prettier is to stop all the ongoing debates over styles.

@dougthor42
Copy link
Contributor

I'm in the "do nothing" camp.

Duplicating the capabilities of isort and other tools, or even just integrating them directly, seems like a waste of effort. I'm sure there's enough to do with simply keeping up with new language features and with supporting backwards compatibility.

Note that I'm not opposed to unsafe things like isort (though I prefer reorder-python-imports, haha) or docstring formatters or warning about possibly useless f-strings. I just think that keeping those separate, outside of black, is the right way to go.

@CiderMan
Copy link
Contributor

CiderMan commented Jun 7, 2022

I also agree with @Zac-HD

However, I do see value in permitting a general opt-out for AST changing translations. Perhaps:

  • --extra-safe to run in AST-preserving mode
  • --experimental to run with AST-changing translations that are only beta quality
    I'm undecided whether this is just --preview or something slightly different
  • Default is to run with all AST-changing translations that are mature enough to be considered 'safe'

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 not (x in y) translation, for example, absolutely should be part of the default behaviour.

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.

@felix-hilden
Copy link
Collaborator

--experimental would overlap with --preview, so I'm not sure if it's worth it.

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 --extra-safe would address that, although personally I'd be okay with Black leaving docstrings alone completely. Not sure if we can backtrack on it though. Formatting string type hints seems okay, since it's more similar to code than free-form documentation.

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: --extra-safe, "default behavior" and "outside the scope of Black". Or adding --all-the-way for more controversial changes if we decide to make them.

@CiderMan
Copy link
Contributor

CiderMan commented Jun 9, 2022

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 f"{{example}}" be changed to "{example}" or not) it should be added to Black. If it is agreed to implement the change, that should be done unconditionally - which brings me to the command line options.

With --extra-safe, no AST-changing translations are enabled so the risk of these changes is low. In this case, we support --preview to include "Release Candidate" changes on top of the released changes

For AST-changing translations, some will be low enough risk to go straight to --preview but, if we are worried that removing useless f-strings might break some code, we might want to initially add it only if --experimental/--all-the-way/--the-full-monty is provided. I think that the key thing is that this should only be a temporary situation - it allows some initial testing of these translations in real-world applications; it is not a different formatting option. The translation may be changed... or removed... or promoted to --preview RC status but it should not stay in this state forever. This mode should not be a hiding place for optional translations.

All IM<HO, of course! 🤣

@felix-hilden
Copy link
Collaborator

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 --preview, so that we don't need a progression of flags from "alpha" to "RC", but perhaps some features are so contentious that it would be appropriate.

At this point it would be really nice to hear from other maintainers as well!

@cooperlees
Copy link
Collaborator

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 --preview and our once a year format changing promise going forward. I'd like to keep it simple as possible and I feel this process does. I do not wish to support and have shades of black.

@tysonclugg
Copy link

tysonclugg commented Jul 26, 2022

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.

@tysonclugg
Copy link

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:

Although automobiles had been produced from the 1880s, until the Model T was introduced in 1908, they were mostly scarce, expensive, and often unreliable. Positioned as reliable, easily maintained, mass-market transportation, the Model T was a great success.

Auto-formatters that change the AST will always be inherently unreliable.

@felix-hilden
Copy link
Collaborator

felix-hilden commented Aug 24, 2022

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:

  • Keep formatting docstrings but provide an AST-safe toggle
  • Revert docstring changes but continue unsafe formatting keeping non-configurability, and maybe advertise docformatter for easier discovery

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.

@Zac-HD
Copy link
Contributor

Zac-HD commented Aug 25, 2022

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.

@felix-hilden
Copy link
Collaborator

After discussing with the team internally, we've decided to:

  • Not introduce new configuration for AST safety or add shades
  • Continue formatting docstrings and making improvements there

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 felix-hilden unpinned this issue Aug 29, 2022
@felix-hilden felix-hilden added R: rejected This will not be worked on and removed S: needs discussion Needs further hashing out before ready for implementation (on desirability, feasibility, etc.) labels Aug 29, 2022
This was referenced Aug 29, 2022
@kaddkaka
Copy link

@felix-hilden Hi, could you up elaborate on what the "advantages of us formatting docstrings" are?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R: rejected This will not be worked on T: enhancement New feature or request T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests