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

Ignore the open parentheses when measuring BestFitsParenthesize #8940

Closed
wants to merge 3 commits into from

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Dec 1, 2023

Summary

This PR changes the definition of BestFitParenthesize to only consider whether its content fits when parenthesizing and not whether the whole parenthesized expression fits.

The difference is subtle but relevant when what comes before BestFitParenthesize already exceeds the configured line width:

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = aaaaaa

The this_is_a... exceeds the configured line width. The existing implementation doesn't parenthesize aaaaa because the opening ( doesn't fit on the line. This is unfortunate, because parenthesizing would make aaaaa fit in the configured line width and is what Black does:

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = (
    aaaaaa
)

This PR implements Black's behavior by ignoring whether there's sufficient space to place the ( and instead only measures whether its content (aaaaaa) fits when indenting.

Test Plan

The similarity index remains unchanged.

project similarity index total files changed files
cpython 0.75804 1799 1648
django 0.99984 2772 34
home-assistant 0.99955 10596 213
poetry 0.96208 317 35
transformers 0.99967 2657 324
twine 1.00000 33 0
typeshed 0.99980 3669 18
warehouse 0.99976 654 14
zulip 0.99957 1459 36

@MichaReiser MichaReiser added the formatter Related to the formatter label Dec 1, 2023
- 3
- ) # type: int
+ another_really_really_long_element_with_a_unnecessarily_long_name_to_describe_what_it_does_enterprise_style = 3 # type: int
+ 3 # type: int
Copy link
Member Author

Choose a reason for hiding this comment

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

The style here is different because black doesn't inline the type: comment. Changing the comment to e.g. ctype matches Ruff's formatting

@@ -123,14 +123,16 @@ def f(
-z: Short | Short2 | Short3 | Short4 = 8
-z: int = 2.3
-z: int = foo()
+z: Loooooooooooooooooooooooong | Loooooooooooooooooooooooong | Loooooooooooooooooooooooong | Loooooooooooooooooooooooong = 7
+z: Loooooooooooooooooooooooong | Loooooooooooooooooooooooong | Loooooooooooooooooooooooong | Loooooooooooooooooooooooong = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Requires other preview styles to match black

)
@@ -35,10 +29,8 @@
-this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = function(
Copy link
Member Author

Choose a reason for hiding this comment

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

Requires other preview styles to match Black

@@ -165,7 +165,9 @@ def foo():
yield (e) # Too many parentheses
# comment

for ridiculouslylongelementnameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee in l:
for ridiculouslylongelementnameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee in (
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't match black although black's formatting here is very peculiar

for ridiculouslylongelementnameeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee in (l):
    pass

It ends parenthesizing l but without splitting the line. IMO, both styles are not ideal but I prefer ours for consistency.

Comment on lines +118 to +122
type Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx = (
int
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a divergence from Black. Black doesn't parenthesize the right side. I don't see why and recommend keeping it because I don't see why type aliases should be different from other assignments.

Copy link
Contributor

github-actions bot commented Dec 1, 2023

ruff-ecosystem results

Formatter (stable)

ℹ️ ecosystem check detected format changes. (+4 -2 lines in 1 file in 41 projects)

latchbio/latch (+4 -2 lines across 1 file)

latch/idl/core/interface.py~L58

     var: Variable
     """+required Variable. Defines the type of the variable backing this parameter."""
 
-    behavior: "Optional[typing.Union[ParameterBehaviorDefault, ParameterBehaviorRequired]]" = None
+    behavior: "Optional[typing.Union[ParameterBehaviorDefault, ParameterBehaviorRequired]]" = (
+        None
+    )
 
     def to_idl(self) -> pb.Parameter:
         return merged_pb(pb.Parameter(var=self.var.to_idl()), self.behavior)

Formatter (preview)

ℹ️ ecosystem check detected format changes. (+4 -2 lines in 1 file in 41 projects)

latchbio/latch (+4 -2 lines across 1 file)

ruff format --preview

latch/idl/core/interface.py~L58

     var: Variable
     """+required Variable. Defines the type of the variable backing this parameter."""
 
-    behavior: "Optional[typing.Union[ParameterBehaviorDefault, ParameterBehaviorRequired]]" = None
+    behavior: "Optional[typing.Union[ParameterBehaviorDefault, ParameterBehaviorRequired]]" = (
+        None
+    )
 
     def to_idl(self) -> pb.Parameter:
         return merged_pb(pb.Parameter(var=self.var.to_idl()), self.behavior)

@MichaReiser MichaReiser marked this pull request as ready for review December 1, 2023 04:26
@MichaReiser
Copy link
Member Author

MichaReiser commented Dec 1, 2023

Hmm, obviously, it is more complicated than this 🤯

Black does not parenthesize the right side here (for reasons?)

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = (
    function(arg1, arg2, arg3)
)

Although function is clearly over the configured width and parenthesizing helps make it fit. Instead, black picks

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = function(
    arg1, arg2, arg3
)

Ultimately, the vertical spacing is about the same, although Black's layout requires considerably more vertical spacing. However, it avoids one pair of parentheses, which is something Black generally optimizes for.

Which makes me wonder what their rule is.

@MichaReiser
Copy link
Member Author

MichaReiser commented Dec 1, 2023

Okay this is interesting. Black does the exact opposite if the right hand side is a function

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_i = (
    function([1, 2, 3], arg1, [1, 2, 3], arg2, [1, 2, 3], arg3)
)

this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = function(
    [1, 2, 3], arg1, [1, 2, 3], arg2, [1, 2, 3], arg3
)

The ( fits for the first example, which is why Black parenthesizes the entire function (at least it seems). The ( doesn't fit for the second example, which is why Black avoids parenthesizing the function.

I compared the similarity index for Poetry using either layout, and it remained unchanged. This feels edge-casey enough that I feel it isn't worth spending too much time on. However, I'm undecided which version I prefer:

  • Always parenthesizing if it makes the content fit gets the result closer to the ultimate goal of fitting code into the configured line width. It's also closer to how our formatter works overall, which generally eagerly parenthesizes expressions. For example, this gets always parenthesized this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = a + b

  • Avoiding the parentheses if even the ( doesn't fit results in fewer parentheses overall, which is a secondary goal. It can be slightly more readable if you e.g. have

    this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = function(
        [1, 2, 3],
        arg1,
        [1, 2, 3],
        arg2,
        [1, 2, 3],
        arg3,
        dddddddddddddddddddd,
        eeeeeeeeeeeeee,
    )
    # instead of
    this_is_a_ridiculously_long_name_and_nobody_in_their_right_mind_would_use_one_like_it = (
        function(
            [1, 2, 3],
            arg1,
            [1, 2, 3],
            arg2,
            [1, 2, 3],
            arg3,
            dddddddddddddddddddd,
            eeeeeeeeeeeeee,
        )
    )

Would love to hear some more opinions on this

@MichaReiser MichaReiser force-pushed the align-type-alias-with-assignment-formatting branch from 7d948b2 to 8fbb3c8 Compare December 4, 2023 05:15
Base automatically changed from align-type-alias-with-assignment-formatting to main December 4, 2023 05:27
@MichaReiser MichaReiser deleted the best-fit-content-only branch January 16, 2024 10:03
@MichaReiser MichaReiser restored the best-fit-content-only branch June 10, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants