search

psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
39.16k stars 2.47k forks source link

Setting the 2024 stable style #4042

Closed hauntsaninja closed 9 months ago

hauntsaninja commented 1 year ago

See issue for 2023 here: https://github.com/psf/black/issues/3407

Here's what this issue is:

Here's the current call to action:

For many of the changes, diff-shades logs have expired. We should find a way to keep these around for longer!

It's currently November, so there's still time to make more uncontroversial changes and hopefuly iron out some issues in the not uncontroversial ones.

Not uncontroversial

string_processing

This one is controversial, vast majority of preview style issues are related to string processing Open issues I believe are caused by string_processing: #2334, #2314, #3466, #3497, #3409, #3424, #3428, #3210, #3410, #3747, #3665, #3663, #2553, #3855, #3802

hug_parens_with_braces_and_square_brackets (#3964, #3992)

Causes O(10000) changes, although counting lines changed is unfavourable to this change. A very substantial change in style for Black. We can't revert once it's in stable. Outstanding crash: #4036

parenthesize_conditional_expressions (#2278)

Causes O(1000) changes. Need to look at diff ~Has unfortunate interactions with comments: #3555~

wrap_long_dict_values_in_parens (#3440)

Causes O(100) changes. Need to look at diff ~Outstanding crash: #3758~ Report of worse splitting: #3452 ~Unnecessary nesting: #4129~ Removes parentheses: #4158

multiline_string_handling (#1879)

Causes O(1000) changes. Need to look at diff Maybe has some synergy with hug_parens_with_braces_and_square_brackets. Unfortunate white space preservation: #4159

dummy_implementations (#3796)

I can't find up-to-date diff-shades, but maybe O(1000) changes Requires a change in flake8 config: #3887 Can result in an extra line, but no change expected to be made: #3872

prefer_splitting_right_hand_side_of_assignments (#3368)

Causes O(100) changes.

hex_codes_in_unicode_sequences (#2916)

Causes O(100) changes Open issue whether lowercase or uppercase is better #3568

allow_empty_first_line_before_new_block_or_comment (#3967)

Causes O(1) changes. This change itself is fine, but it's related to a controversial and churn causing change in 2023 style. I think we should probably revert that 2023 change; I posted a longer issue about this over here https://github.com/psf/black/issues/4043

parenthesize_long_type_hints (#3899)

Causes O(100) changes

module_docstring_newlines (#3932)

Causes O(100) changes

Uncontroversial

respect_magic_trailing_comma_in_return_type (#3916)

O(10) changes. Shouldn't affect existing formatting too much, unless there's a buggy trailing comma All changes in diff-shades are incidental, but are good and what you'd expect Black to do

no_blank_line_before_class_docstring (#3692)

O(100) changes. Makes consistent with function behaviour Blank lines have been a little controversial, but I think this should be totally fine

wrap_multiple_context_managers_in_parens (#3489)

O(100) changes. Naively, looks great to me

fix_power_op_line_length (#3942)

O(10) changes. A bug fix

add_trailing_comma_consistently (#3393)

O(1) changes. A bug fix

skip_magic_trailing_comma_in_subscript (#3209)

O(1) changes. A bug fix, affects a non-default option

blank_line_after_nested_stub_class (#3564)

Only affects stubs, adds some blank lines in an uncommon case

blank_line_between_nested_and_def_stub_file (#3862)

Only affects stubs, adds some blank lines in even less common case

accept_raw_docstrings (#3947)

O(1) changes. Just a bug fix

long_case_block_line_splitting (#4024)

0 changes. Seems strictly good

walrus_subscript (#3823)

0 changes. Straightforward style change

improved_async_statements_handling (#3609)

0 changes. More consistent

single_line_format_skip_with_multiple_comments (#3959)

0 changes. Makes life better

hauntsaninja commented 1 year ago

My vibes:

AlexWaygood commented 1 year ago

My ¢2 as a Black user:

  1. I generally agree with @hauntsaninja's vibes/analysis overall!
  2. I ran Black with --preview on a few projects, and in those projects, I'm sad to say that I dislike most of the string-formatting-related changes the preview style make.

  3. I really, really like hug_parens_with_braces_and_square_brackets and hope it gets in! One of my biggest gripes with black is how annoying its formatting of multiline frozensets etc. has been in the past, and this addresses that. The one thing I'd say is that it surprised me that the preview style does this:

    -    return AnnotationStats(
-        *[random.randint(0, 1000) for _ in AnnotationStats.__annotations__]
-    )
+    return AnnotationStats(*[
+        random.randint(0, 1000) for _ in AnnotationStats.__annotations__
+    ])

    I prefer the non-preview style there -- I believe this is basically the same thing @JelleZijlstra's is saying in https://github.com/psf/black/pull/4012#discussion_r1384284154

JelleZijlstra commented 1 year ago

Thanks, this is great!

We should try to make a 24.1a1 soon that implements a draft stable style, so we can get more feedback.

I haven't thought too hard about each change, but a few thoughts:

yilei commented 1 year ago

On string_processing:

We have been using it internally (Google) since the beginning of this year and it has been working well for us. So I'm motivated to keep this and help graduate it to the stable style (too late for the 2024 stable style though).

Possibly we can keep a more limited version, e.g. only merging adjacent string literals that are on the same line.

This is a good strategy, I think we can split string_processing into a few separate style changes. Moving all of them at once is hard to reason about and is too disruptive.

tungol commented 1 year ago

I tested preview on a code base of mine, and it mostly looked good. The only change I was really surprised by was from string processing; the same issue as https://github.com/psf/black/issues/3663 and https://github.com/psf/black/issues/3210 . A little testing showed that it's from the string_paren_strip transformer. I like the idea of that part of string_processing, but that's a pretty ugly downside. It seems like it's stripping the parentheses and then re-inserting them at a worse place, which is consistent behavior with non-preview style if I manually remove the parentheses first. Not strictly a problem with string_paren_strip, I guess, but not great.

For hug_parens, I like it in theory but it does feel less consistent than the current behavior, at least in some cases. I'm not totally sure about it. I can understand why you'd compare it and multiline_string_handling, but I don't see any issues with the latter - that one looks great to me.

I think that parenthesize_conditional_expressions is a big improvement for conditionals in the original context of function arguments, but I don't think I like what it's doing in other contexts.

JelleZijlstra commented 12 months ago

I started putting together a PR to prepare an alpha for the 2024 stable style, and here's a few thoughts:

DanielNoord commented 11 months ago

Is there a chance of module_docstring_newlines making it in? As far as I can see there has only been support for the feature since it was first requested in 2020 (https://github.com/psf/black/issues/1872) and I haven't seen issues about it being opened since it was added to preview.

If it means anything it was also quickly picked up by ruff in https://github.com/astral-sh/ruff/issues/7995 without any discussion about it being potentially controversial.

It seems to me to be controversial as it would create a lot of diffs across projects, but doesn't change styling in any way that users would say "hey, what's black doing there". I would really like to see this change make it in to standardize these newlines across the projects I work on (and which don't want to run with preview style).

JelleZijlstra commented 11 months ago

@DanielNoord yes, that one is expected to go in (unless either lots of people speak up and say they don't like it, or we discover bugs caused by it).

MichaReiser commented 11 months ago

I'm still writing it up but we track Ruff's preview style compatibility in https://github.com/astral-sh/ruff/issues/8678 Ruff's stable style already includes some of the mentioned uncontroversial preview styles.

Regarding multiline_string_handling. Ruff stable already formats 

MULTILINE = """
foobar
""".replace("\n", "")

and we've generally received positive feedback. I haven't yet come around to implement the special casing inside call expressions. So I can't say how people feel about that.

I personally like hug_parens_with_braces_and_square_brackets that that's probably because I'm biased from Prettier ;)

Ruff formats long dict values slightly different from black (wrap_long_dict_values_in_parens). Parenthesizing the values would align the behavior. Although it sets a new precedence of removing parentheses from sub-expression, something Black so far very explicitly avoids (in comparison to Prettier that normalises all parentheses). There's a similar issue around slice indexes that may benefit from parenthesizing as well https://github.com/astral-sh/ruff/issues/7316

I'll share more feedback when I had time to take a closer look at the changes and thanks for drafting this up, it helps to understand how Black's styleguide will change.

takanakahiko commented 11 months ago

Release notes states the following regarding #3445 :

Please try out the preview style with black --preview and tell us your feedback. All changes in the preview style are expected to become part of Black’s stable style in January 2024.

Isn't this a topic for this discussion?

JelleZijlstra commented 11 months ago

@takanakahiko I'm not sure exactly what you're asking. This issue is definitely the right place to discuss what should go into the 2024 stable style. The default is that everything in the preview style will move to stable, unless there's some reason not to. As discussed above, string processing (which is what #3445 affects) probably will not go into the 2024 stable style because there are many open bugs around it.

takanakahiko commented 11 months ago

@JelleZijlstra I may have had a problem with my English, sorry.

I thought all Preview styles were listed because it said the following.

I've listed all the features we have enabled under --preview

However, I was concerned that my interest #3445 was not listed, so I asked my question.

Thanks for the answer. I understand that it will not be introduced in the stable style because it is related to the string processing issue.

jakkdl commented 11 months ago

I started putting together a PR to prepare an alpha for the 2024 stable style, and here's a few thoughts:

* The test suite found a new crash with `parenthesize_long_type_hints`: [Crash with preview style + line length 1 #4062](https://github.com/psf/black/issues/4062)

Fixed the crash, feel free to tag me if any other issues with either of the type-hint features I added cause any other problems :)

Fatal1ty commented 11 months ago

Here is my feedback. TLDR: I like the changes with --preview flag more than without it.

I was testing black 23.11.0 with --preview on https://github.com/Fatal1ty/mashumaro

I like this idea with Ellipsis on the same line (there is no longer a need to exclude it from coverage)

-    def decode(self, data: Any) -> T:  # pragma: no cover
-        ...
+    def decode(self, data: Any) -> T: ...

It found an unnecessary string concatenation:

         raise ValueError(
-            f"Time zone {s} must be either UTC " f"or in format UTC[+-]hh:mm"
+            f"Time zone {s} must be either UTC or in format UTC[+-]hh:mm"
         )

This became more readable:

         schema = JSONObjectSchema(
-            additionalProperties=_get_schema_or_none(
-                instance.derive(type=args[1]), ctx
-            )
-            if args
-            else None,
-            propertyNames=_get_schema_or_none(
-                instance.derive(type=args[0]), ctx
-            )
-            if args
-            else None,
+            additionalProperties=(
+                _get_schema_or_none(instance.derive(type=args[1]), ctx)
+                if args
+                else None
+            ),
+            propertyNames=(
+                _get_schema_or_none(instance.derive(type=args[0]), ctx)
+                if args
+                else None
+            ),
         )

Special thanks for this change, I like it a lot:

     serialize_by_alias: Union[
-        bool, Literal[Sentinel.MISSING]
-    ] = Sentinel.MISSING
-    namedtuple_as_dict: Union[
-        bool, Literal[Sentinel.MISSING]
+        bool, Literal[Sentinel.MISSING], float, List[str]
     ] = Sentinel.MISSING
+    namedtuple_as_dict: Union[bool, Literal[Sentinel.MISSING]] = (
+        Sentinel.MISSING
+    )

What interesting is that ruff turns all this changes back, so all the credits go to black!

MichaReiser commented 11 months ago

Some feedback to the most recent changes to hug_parens_with_braces_and_square_brackets after having implemented some of them in Ruff. I'm not sure if this is the right place to discuss these. Let me know if not and I'll move my comments.

It seems that Black now recursively collapses (hugs) parentheses. We believe that this makes the code harder to read. Like here, it becomes difficult to tell which parentheses belong together

                                     )
                                 )
                                 if commented_entity:
-                                    entity_map = CommentedMap(
-                                        [(entity["entity"], entity["value"])]
-                                    )
+                                    entity_map = CommentedMap([(
+                                        entity["entity"],
+                                        entity["value"],
+                                    )])
                                     entity_map.yaml_add_eol_comment(
                                         commented_entity, entity["entity"]
                                     )
                                     entities.append(entity_map)
                                 else:
                                     entities.append(
-                                        OrderedDict(
-                                            [(entity["entity"], entity["value"])]
-                                        )
+                                        OrderedDict([(
+                                            entity["entity"],
+                                            entity["value"],
+                                        )])
                                     )
                     else:
                         entities.append(

But maybe that's just us not being used to that style.

We were surprised to learn that Black now leaves the following snipped as is

self._edits.append(
    {"operation": "update", "id": index, "fields": list(fields), "region_fields": []}
)

and doesn't format it to 

self._edits.append({
    "operation": "update", "id": index, "fields": list(fields), "region_fields": []
})

We prefer the second style because it leads to fewer changes when the length of the dictionary changes:

self._edits.append({
    "operation": "update", 
    "id": index, 
    "fields": list(fields), 
    "region_fields": [], 
    "loooger": "more",
})

The parentheses and the first entry remain unchanged. Making it easier to spot the differences

self._edits.append({ "operation": "update", "id": index, "fields": list(fields)})

Again, the opening parentheses remains unchanged.

It also feels more consistent with function calls where the parentheses (for good reasons) are kept on the first line:

self._edits.append(
    "operation", "update", "id", index, "fields", list(fields), "region_fields", []
)

What interesting is that ruff turns all this changes back, so all the credits go to black!

Ruff doesn't implement Black's preview style yet. But yes, all credit for Ruff's style guide goes to Black, because Ruff follows Black's style guide.

JelleZijlstra commented 11 months ago

Replying to a few recent posts (including my own):

hex_codes_in_unicode_sequences: I think there are good reasons to prefer lowercase. I would propose to keep this out of the 2024 stable style and make the 2024 preview style use lowercase.

I misread something here, we already use lowercase. We should keep this in the 2024 style.

@Fatal1ty

I like this idea with Ellipsis on the same line (there is no longer a need to exclude it from coverage)

This actually looks concerning to me, because Black shouldn't go around removing comments. I can't reproduce this behavior, though.

@MichaReiser thanks for the feedback! I think we'll exclude the hug_parens changes from the stable style this time because they came in late of the year and are likely to be controversial. Could you perhaps make a new issue?

jakkdl commented 11 months ago

Replying to a few recent posts (including my own):

@Fatal1ty

I like this idea with Ellipsis on the same line (there is no longer a need to exclude it from coverage)

This actually looks concerning to me, because Black shouldn't go around removing comments. I can't reproduce this behavior, though.

I'm pretty sure they meant that they removed the comment themselves.

Fatal1ty commented 11 months ago

Replying to a few recent posts (including my own): @Fatal1ty

I like this idea with Ellipsis on the same line (there is no longer a need to exclude it from coverage)

This actually looks concerning to me, because Black shouldn't go around removing comments. I can't reproduce this behavior, though.

I'm pretty sure they meant that they removed the comment themselves.

Yes, you're right. Black didn't remove the comment, I did.

MichaReiser commented 11 months ago

I plan to create PRs for larger projects using Ruff's formatter and ask them for feedback on the preview style once I've implemented all preview styles (some of them are tricky to get right!). It will probably be January until I find the time but I'll share the feedback here if you're interested (and/or maybe that's something you want to do as well)?

JelleZijlstra commented 11 months ago

Thanks, that's great! I won't have much time until January. I'm planning to put out an alpha release today or tomorrow with the new style, then finish things up in January.

yuhrichard commented 11 months ago

Hello, we’re engineers from Lyft and want to provide some initial feedback from testing out the new features from the alpha (https://github.com/psf/black/releases/tag/24.1a1):

Bulk of the observed changes have been parenthesizing conditional expressions, splitting RHS of assignments, wrapping long dict values in parentheses, and docstring formatting which have looked good overall and are big improvements in readability. From the changelog, it shows that parenthesizing conditionals may or may not be included, but so far, we definitely liked this feature overall.

We found this case where black added parentheses around a dictionary (after topLevelBase and secondaryBase), causing some extra nesting that seemed unnecessary, although it’s still overall readable (playground link):

Wanted to confirm if this formatting is due to wrap_long_dict_value_in_parens?

# In
class Random:
    def func():
        random_service.status.active_states.inactive = (
            make_new_top_level_state_from_dict(
                {
                    "topLevelBase": {
                        "secondaryBase": {
                            "timestamp": 1234,
                            "latitude": 1,
                            "longitude": 2,
                            "actionTimestamp": Timestamp(
                                seconds=1530584000, nanos=0
                            ).ToJsonString(),
                        }
                    },
                }
            )
        )

# Out: Black formatted
class Random:
    def func():
        random_service.status.active_states.inactive = (
            make_new_top_level_state_from_dict({
                "topLevelBase": (
                    {
                        "secondaryBase": (
                            {
                                "timestamp": 1234,
                                "latitude": 1,
                                "longitude": 2,
                                "actionTimestamp": (
                                    Timestamp(
                                        seconds=1530584000, nanos=0
                                    ).ToJsonString()
                                ),
                            }
                        )
                    }
                ),
            })
        )

Also, is there a sense when to expect the major release/2024 stable style (ie) mid or end of Jan)? We’ll continue to do some more testing and provide additional feedback towards the end of the year. Thanks for all of the development work!

cgohlke commented 11 months ago

In this case the trailing/inline comment is moved to the wrong line:

a = "".join(
    (
        "",  # comment
        "" if True else "",
    )
)

becomes

a = "".join((
    "",
    "" if True else "",  # comment
))
hauntsaninja commented 10 months ago

@yuhrichard Thanks for the feedback! Yes, the behaviour you're seeing is from wrap_long_dict_value_in_parens. I've filed https://github.com/psf/black/issues/4129 for your issue. I'm not sure when exactly the release would happen, but I'd definitely guess second half of January or later

@cgohlke Thanks for the report! I think this is the same issue as https://github.com/psf/black/issues/3555 , relates to the parenthesize_conditional_expressions style

AlexeyDmitriev commented 10 months ago

I really like hug_parens_with_braces_and_square_brackets, hope it gets merged in 2024 so that we don't have to wait a year. It really made code look better in all cases I saw.

As for multiline string processing, I am not a big fan, I find implicit concatenation of string literals somewhat confusing and would prefer single (but long) literal.

One case where I find it actively worse then it were:

Original:

assert x == "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo o oo  o oo o oo  o o o o o o oooo oo o oo ng text"

Old black: (clunky but ok)

assert (
    x
    == "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo o oo  o oo o oo  o o o o o o oooo oo o oo ng text"
)

New black (with preview): == is so unnoticable here

assert (
    x
    == "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo"
    " o oo  o oo o oo  o o o o o o oooo oo o oo ng text"
)

Ideal result (with new hugging style) considering we need to have splitting

assert x == (
    "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo"
    " o oo  o oo o oo  o o o o o o oooo oo o oo ng text"
)

Probably good enough is this:

assert (
    x == (
        "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo"
        " o oo  o oo o oo  o o o o o o oooo oo o oo ng text"
    )
)

Also, I think multiline string should almost always be in parens (except in cases where it's already in cases when they are already in parens from the function call. There are cases when it's not this way (in asserts as above), in list comprehensions:

original:

lst = [
    "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo o oo  o oo o oo  o o o o o o oooo oo o oo ng text"
    for line in range(5)
]

black with preview

 lst = [
    "looooooooooooooooooooooooooooooo o o o oo o o oo oo o o oo o oo o oo ooo o o oo o"
    " oo  o oo o oo  o o o o o o oooo oo o oo ng text"
    for line in range(5)
]
phlogistonjohn commented 10 months ago

I hope this is the correct place to comment on this. I "accidentally" ran the alpha version on my project and I think the new dummy_implementations rule made the following change(s) (slightly simplified to highlight a certain type of change):

# Before
class Parser(typing.Protocol):
    def set_defaults(self, **kwargs: typing.Any) -> None:
        ...  # pragma: no cover

# After
class Parser(typing.Protocol):
    def set_defaults(
        self, **kwargs: typing.Any
    ) -> None: ...  # pragma: no cover

This doesn't seem to me to be much of an improvement in readability. I think I understand the intent of the rule to move the elipsis onto the same line as the def but doing so at the expense of then having to split the definition across multiple lines seems much less pleasant than just leaving it as-is IMO. Thanks for listening!

JelleZijlstra commented 10 months ago

@phlogistonjohn that's issue #3872. As I wrote on the issue, I think it's still a net positive on balance, but there's definitely some risk, especially in cases where there is a comment after the ....

JelleZijlstra commented 10 months ago

@AlexeyDmitriev string splitting won't be in the 2024 stable style.

(And for everyone commenting, thanks for your feedback!)

AlexeyDmitriev commented 10 months ago

@JelleZijlstra thanks for your feedback. What is the expected time when black 2024.* is published?

AlexeyDmitriev commented 10 months ago

@JelleZijlstra I was looking at #4106, Do I understand correctly that parens hugging will not make it either?

JelleZijlstra commented 10 months ago

We'll publish it by the end of the month and the hugging changes will not be included.

amyreese commented 10 months ago

Sorry if these pieces have already been covered, but I ran the preview style (from 24.1a1) over a small portion of Meta's codebase and have some feedback. Most of the style changes seem like a positive change, but I think the changes to collapsing/compacting elements and especially multiline strings is a net-negative.

First some changes I'm happy to see:

# before
for (a, b) in things:
    ...

# after
for a, b in things:
    ...
# before
value: Optional[
    int
] = None  # some really long comment

# after
value: Optional[int] = (
    None  # some really long comment
)

Love it!

# before
func(
    arg1,
    "long string"
    if condition
    else "other string",
)

# after
func(
    arg1,
    (
        "long string"
        if condition
        else "other string"
    ),
)

Much clearer, exactly how I would have rewritten it myself! 💜

Now, some examples where I think the changes result in less-readable code, particularly where black takes aligned triple quotes and then unaligns them:

# before 
Thing(
    """
    value
    """
)

# after
Thing("""
    value
    """)

or worse:

# before
VALUE = [
    Thing(
        """
        value
        """
    )
]

# after
VALUE = [Thing("""
    value
    """)]
# before
func(
    (
        value
        + """
        something
        """
    ),
    argument,
)

# after
func(
    (value + """
        something
        """),
    argument,
)

Not pretty, and not really any more readable than before? ¯\(ツ)/¯

Is that related to the "paren hugging" you mentioned, or is this still scheduled for the 2024 style?

amyreese commented 10 months ago

Is that related to the "paren hugging" you mentioned

Maybe not, because I just found this change as well:

# before
def foo():
    result = (
        """
        something
        """
        + value
    )

# after
def foo():
    result = """
        something
        """ + value

Distinctly less readable IMO.

JelleZijlstra commented 10 months ago

Update: