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

Setting the 2023 stable style #3407

Closed
JelleZijlstra opened this issue Dec 9, 2022 · 17 comments · Fixed by #3418
Closed

Setting the 2023 stable style #3407

JelleZijlstra opened this issue Dec 9, 2022 · 17 comments · Fixed by #3418
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases help wanted Extra attention is needed T: enhancement New feature or request T: style What do we want Blackened code to look like?

Comments

@JelleZijlstra
Copy link
Collaborator

JelleZijlstra commented Dec 9, 2022

Hello! Please read the following before commenting...

If you're here to learn more about the proposed 2023 stable style or provide feedback, you're in the right place.

Here's what you need to know:

  • This issue contains a list of all of the changes in the current preview style. Except for the ones crossed out, they will promoted to the 2023 stable style in 23.1.0.
  • This issue is meant to collect feedback and concerns about the proposed stable style for 2023. You're welcome to suggest tweaks to a pre-existing change, removing a change, or even a new change (although don't hold your breath on it being promoted to the 2023 stable style, it will probably have to wait until 2024).
  • If the discussion on particular change / item gets too long, we will split it into a separate issue. These issues will be linked and highlighted.
  • If you have feedback on the current 2023 preview style (and aren't suggesting to promote something from it in 2023), please open a new issue and make it clear it's about the preview style only. While we welcome all feedback, this issue is focused on the proposed 2023 stable style.

To install the latest draft of the 2023 style for testing, please run pip install black==23.1a1.

If you would like a (more digestible) summary about what's been changed, @ichard26 wrote a blog post to go along with 23.1a1.

Thank you! ~ the maintainer team


Original description

The first 2023 release is coming up, which means we'll have to decide which parts of the current preview style should go into the 2023 stable style, per our stability policy.

Here's how I propose we go about it:

  • In this issue, I'll list all style changes in the current preview style
  • We'll default to promoting every part of the preview style to the stable style. However, we will not promote any feature that causes known crashes or that causes new bugs where we make lines longer than the length limit.
  • Anyone can comment here if they have concerns about a particular part of the new stable style. However, if the discussion on any specific feature gets too long, we should split it into a separate linked issue.
  • The maintainer team will make a final call on what goes into the stable style, sometime before the first January release.

I put "(!)" in front of changes that seem more potentially controversial. List of changes:

  1. --skip-string-normalization / -S now prevents docstring prefixes from being normalized as expected (Actually disable docstring prefix normalization with -S + fix instability #3168) (since 22.8.0)
  2. When using --skip-magic-trailing-comma or -C, trailing commas are stripped from subscript expressions with more than 1 element (Strip trailing commas in subscripts with -C #3209) (22.8.0)
  3. (!) Implicitly concatenated strings inside a list, set, or tuple are now wrapped inside parentheses (Add parens around implicit string concatenations where increases readability #3162) (22.8.0) (only relevant with ESP enabled)
  4. Fix a string merging/split issue when a comment is present in the middle of implicitly concatenated strings on its own line (Fix a string merging/split issue caused by standalone comments. #3227) (22.8.0) (only relevant with ESP enabled)
  5. (!) Docstring quotes are no longer moved if it would violate the line length limit (Docstring too long (issue #1632 fix) #3044) (22.6.0) see also style regression: too long single line docstrings have their quotes moved to a new line. #3320
  6. Parentheses around return annotations are now managed (Return annotation brackets #2990) (22.6.0) see Preview style incorrectly removes parens from walrus in annotation #3419
  7. Remove unnecessary parentheses around awaited objects (Remove redundant parentheses around awaited coroutines/tasks #2991) (22.6.0)
  8. Remove unnecessary parentheses in with statements (Remove unnecessary parentheses from with statements #2926) (22.6.0)
  9. (!) Remove trailing newlines after code block open (Remove newline after code block open #3035) (22.6.0)
  10. Code cell separators #%% are now standardised to # %% (dont skip formatting #%% #2919) (22.3.0)
  11. Remove unnecessary parentheses from except statements (Remove unnecessary parentheses from except clauses #2939) (22.3.0)
  12. Remove unnecessary parentheses from tuple unpacking in for loops (Remove unnecessary parentheses from tuple unpacking in for loops #2945) (22.3.0)
  13. Avoid magic-trailing-comma in single-element subscripts (Avoid magic-trailing-comma in single-element subscripts #2942) (22.3.0)
  14. (!!!) Experimental string processing ("ESP") (22.1.0) see also string_processing: Worse formatting of assert #3210, Black --preview mode produces invalid code in the first pass, causing second pass to fail #3117, maximum recursion depth exceeded while calling a Python object #3340, Enable feature string_processing to be default #2188, string_processing: Performance regression #2314, string_processing: reformats to exceed line length and is also ugly #3409
  15. Enforce empty lines before classes and functions with sticky leading comments (Enforce empty lines before classes/functions with sticky leading comments. #3302) (22.12.0)
  16. Reformat empty and whitespace-only files as either an empty file (if no newline is present) or as a single newline character (if a newline is present) (Remove whitespaces of whitespace-only files #3348) (22.12.0)
  17. Implicitly concatenated strings used as function args are now wrapped inside parentheses (Wrap concatenated strings used as function args in parens. #3307) (22.12.0) (only relevant with ESP enabled)
  18. Correctly handle trailing commas that are inside a line's leading non-nested parens (Correctly handle trailing commas that are inside a line's leading non-nested parens #3370) (22.12.0)

Known blocking preview style bugs:

(I got this from the release logs "Preview style" sections, skipping changes that were fixes for previous bugs. I added PR lists, the first version the change was added, and links to relevant issues. Maintainers, feel free to edit to link other relevant issues.)

I encourage anyone to run black --preview on their codebase and flag changes they don't like.

@JelleZijlstra JelleZijlstra added T: enhancement New feature or request C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases labels Dec 9, 2022
@JelleZijlstra
Copy link
Collaborator Author

JelleZijlstra commented Dec 9, 2022

My takes:

For 5. We should fix #3320 and otherwise back out #3044. The current behavior is ugly.

For 14. Experimental string processing is nice but we can't move it into stable unless #2314, #3117, and #3340 are fixed.

@JacobHayes
Copy link

JacobHayes commented Dec 9, 2022

  1. (!) Implicitly concatenated strings inside a list, set, or tuple are now wrapped inside parentheses (Add parens around implicit string concatenations where increases readability #3162) (22.8.0)

+1

  1. (!) Remove trailing newlines after code block open (Remove newline after code block open #3035) (22.6.0)

+1

  1. (!!!) Experimental string processing (22.1.0) see also ...

I haven't read up on all the linked issues yet and guessing I don't have much new to say, but I don't love the results from this. I probably have a higher tolerance for over-length strings, but generally dislike the implicit concatenation because:

  • it's quite different from what the audience will see (eg: in raise, print, ...) - their output will still be a single line
    • seeing a single line as the user sees it is a useful cue to the author to reword/rewrite the message or not
    • this makes it harder to search for strings - if I copy an error message and do a code search, I may not get a hit depending on what part I copied and how the string was split up (though this still arises elsewhere, like obviously {x} formatting)
  • bugs from concatenation when a new element/arg was intended
    • I know the proposals to add extra parens will help with clarity, but at the cost of 1-2 more paren lines + an indent, which might also require more splits
  • typos from missing spaces at the end of string 1 or beginning of string 2 are harder to check
    • seems black prefers placing them at the beginning, the consistency is nice at least

Those are issues with string concatenation, not necessarily black, but that's why I still try to avoid them.

In my project (full diff - line length is configured to 100), the string changes are the main ones and imo they are less readable, eg:

             raise TypeError(
-                f"Expected {tgt_name}.cast({value}) to return an instance of {tgt_name}, got: {casted}"
+                f"Expected {tgt_name}.cast({value}) to return an instance of {tgt_name}, got:"
+                f" {casted}"
             )
                 raise ValueError(
-                    f"{annotation} cannot be matched to a View, try setting one explicitly (eg: `Annotated[int, arti.views.python.Int]`)"
+                    f"{annotation} cannot be matched to a View, try setting one explicitly (eg:"
+                    " `Annotated[int, arti.views.python.Int]`)"
                 )
             key_component_specs = {
-                f"{name}{self.partition_name_component_sep}{component_name}": f"{{{name}.{component_spec}}}"
+                f"{name}{self.partition_name_component_sep}{component_name}": (
+                    f"{{{name}.{component_spec}}}"
+                )
                 for name, pk in self.key_types.items()
                 for component_name, component_spec in pk.default_key_components.items()
             }

(this one ^ I'm a bit confused about - the old code was over 100 chars but split across the two strings, so I'd think would have already been split by black without --preview)

     with pytest.raises(
         ValueError,
-        match=r"BadProducer.build - the following parameter\(s\) must be defined as a field: {'a1'}",
+        match=(
+            r"BadProducer.build - the following parameter\(s\) must be defined as a field: {'a1'}"
+        ),
     ):

(this one ^ looks fine either way IMO, but not sure why that only changes with --preview - the , is the 101st character)

Perhaps there could be a string-length setting that folks could set separately from line length (eg: I'd set to ~infinity)? A bit against the general preference for low-configuration though.

@yilei
Copy link
Contributor

yilei commented Dec 10, 2022

I think 3 & 17 should be enabled together if they go to stable, to be consistent.

Also, 15-18 should be labeled as 22.12.0 release?

@JelleZijlstra
Copy link
Collaborator Author

@JacobHayes thanks, that's useful feedback. At this point there's still half a dozen stability bugs with experimental string processing, so that alone is enough to block it from going into the 2023 stable style. However, I think we should consider dropping (most of) the feature entirely, instead of leaving it behind a flag forever, since there are so many stability bugs and often it arguably makes for worse output. That should be discussed in #2188, though.

@yilei thanks, fixed. Writing up this list is what prompted me to cut the 21.12.0 release in the first place :)

@olivia-hong
Copy link
Contributor

Hello! Wanted to raise a question around the multiline string formatting fix PR (#1879). For context, our plan at Lyft is to roll out black to all our ~2000 repos early next year, now that it's started shipping stable releases. The multiline formatting bugfix is the last blocking issue we’ve found (given how many of our repos use multiline strings) that we need to resolve before adopting black en masse. When we started working on the PR again in October, we were hoping to get it baked in preview for a few months and maybe squeeze it into the 2023 stable release, to avoid the need to run a fork.

Want to be clear that we're not asking for this fix to be included in this stable release given that it hasn’t even been merged into the repo yet. When we roll out black next year we’d prefer to stick to upstream black (whether that be on stable or with —preview) rather than have to run a fork. Therefore, would love to get a sense of when the PR might get some review/merged and if there is anything we can do to help make that happen!

@JelleZijlstra
Copy link
Collaborator Author

Sorry for dropping the ball on that! I can't make promises but I'll hopefully be able to make time in the next few weeks to review open PRs. If you're able to fix the merge conflicts on that PR, I will take a look.

However, unfortunately I feel it's too late for the 2023 stable style. We can put it in preview in 2023 and then hopefully promote it to stable for 2024.

@olivia-hong
Copy link
Contributor

Really appreciate the update, thank you! Will make sure to get that PR free of merge conflicts

@JelleZijlstra
Copy link
Collaborator Author

I just published a pre-release 23.1a1 with a first draft of the 2023 stable style, based on the PR branch for #3418. Please try it out on your codebase and leave a message here if you have any feedback.

@ichard26 ichard26 added help wanted Extra attention is needed T: style What do we want Blackened code to look like? labels Dec 19, 2022
@ichard26 ichard26 added this to the Release 23.1.0 milestone Dec 19, 2022
@jgillard
Copy link

I've just tested this out with a medium-sized codebase. All as expected apart from this one issue with a comment at the end of a function getting 2 newlines added above it. Removing @decorator1 stops the comment getting moved, as does removing # fmt:skip.

image

https://black.vercel.app/?version=stable&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4ADZAIZdAD2IimZxl1N_WlbvK5V9KEd0suDTtKdXyoIEqib4HPPAOaGeijv-QM5F2CIWMxNomuS8WtfuVtc-CBav7wmp196mwOSJGXUmaRTdIwYr1wf881xD3biQ1s52KZSzb20ApVmRJ9HPRaEOhoVMkb9jDbRHrld5-g2LTBdcrPwjnT90_-ZGPvMAAAAAsri7GeyV3k8AAaIB2gEAAEJ-yuKxxGf7AgAAAAAEWVo=

@JelleZijlstra
Copy link
Collaborator Author

Thanks @jgillard! I opened a new issue #3454 since that's clearly a bug.

@blakeNaccarato
Copy link

How does "Remove trailing newlines after code block open" work for functions with docstrings? Does it remove a newline between docstrings and code, or just normalizes newlines directly after the end of the function signature?

@wookie184
Copy link

The movement of the comment here doesn't seem ideal:

COMPLICATED_REGEX = (  
    "hsfshfsdhfkdsfhfhsdkhjskdfhkdsjfhk" # Important comment to describe what is going on
)

becomes

COMPLICATED_REGEX = (  # Important comment to describe what is going on
    "hsfshfsdhfkdsfhfhsdkhjskdfhkdsjfhk"
)

I think it would be nicer if it was put on a new line, or if not left as is.

COMPLICATED_REGEX = (  
    # Important comment to describe what is going on
    "hsfshfsdhfkdsfhfhsdkhjskdfhkdsjfhk"
)

(v23.1a1)

@JelleZijlstra
Copy link
Collaborator Author

@wookie184 I can't reproduce that with the stable style on 23.1a1:

$ python3.10 -m black -c '''COMPLICATED_REGEX = (  
    "hsfshfsdhfkdsfhfhsdkhjskdfhkdsjfhk" # Important comment to describe what is going on
)'''
COMPLICATED_REGEX = "hsfshfsdhfkdsfhfhsdkhjskdfhkdsjfhk"  # Important comment to describe what is going on

It reproduces with --preview, but that's the ESP feature (no. 14 on the above list), which will not be part of the 2023 stable style.

@wookie184
Copy link

It reproduces with --preview, but that's the ESP feature (no. 14 on the above list), which will not be part of the 2023 stable style.

Ah, I got mixed up, sorry for the noise.

@Zac-HD
Copy link
Contributor

Zac-HD commented Dec 25, 2022

I just published a pre-release 23.1a1 with a first draft of the 2023 stable style, based on the PR branch for #3418. Please try it out on your codebase and leave a message here if you have any feedback.

I found the following bug when parens are incorrectly removed around an await in a with-statement:

async def f():
    # This is odd but valid, bear with me...
    if x := await sleep(0):
        pass
    
    # This is valid syntax, *until* Black removes the parens!
    # OK, `with (await sleep(0)) as x:` would be nicer,
    # but introducing a syntax error is definitely a bug.
    with (x := await sleep(0)):
        pass

https://black.vercel.app/?version=stable&state=_Td6WFoAAATm1rRGAgAhARYAAAB0L-Wj4AG6AQFdAD2IimZxl1N_WlPS-KmmddHYvuhW3vVx6eGobs5-xw96QJUyRji_GnUEHOwF6oEDTWcSPbMxh3NWeBVM5meRnn2G8PmgidhHweXeJt31QehaKrQnBlBQO3nUR84O8giwpejxKvfMkdv-xRdc8qi6LzbHW3CfbpbhRvpZtgfA7X3diOgh1NowyRjGF9NbK9O6zPS5EnjEyliQQ_L5I1aL1qhDVSAykoq5pkjs2f5mu_3CYh6vE3pxYiUxS7Yoi5N_W8fvYdpC49zO2-VMICAs4WQBq1p4gGQvqJHS4lrQlgtlVy3-IijTKckyOk2wVsGJyl6zzm4q2Hcz1PTi2HSmO6c0AAAAAAT4lLKu1CNFAAGdArsDAADi9VjQscRn-wIAAAAABFla


Other than that, I really like it! Lots of deleting blank lines at the start of blocks, less commonly removing parens mostly in for-clauses. The only cases where I intervened were assignments where a trailing comma suddenly had an effect, and I preferred to delete it than split the expression across multiple lines. On the whole, a lovely stable-feeling update!

@olivia-hong
Copy link
Contributor

Tested on a bunch of our repos at Lyft and all the changes looked as expected; didn't run into any bugs. The vast majority of the changes were removing empty lines at the beginning code blocks. All the docstring, tuple, and trailing-comma splitting reformats I saw look more readable than the previous output. Overall seems like a pretty straightforward stable release to update to once it comes out and looking forward to adopting these new features!

@tpvasconcelos
Copy link

Also tested this against a bunch of large internal and public projects and found no issues 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases help wanted Extra attention is needed T: enhancement New feature or request T: style What do we want Blackened code to look like?
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants