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

Improve function declaration wrapping when it contains generic type definitions #4553

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Pedro-Muller29
Copy link

Description

This PR addresses #4071 by improving how Black wraps function definitions that include generic type declarations.

Current Behavior

When a function with generic type declarations is formatted, Black prioritizes wrapping the generic type definitions, even in cases where splitting the arguments would result in a cleaner layout. For example:

Input:

def func[T](a: T, b: T,) -> T:
    return a

Current Output:

def func[
    T
](a: T, b: T,) -> T:
    return a

New Behavior

With this improvement, Black prioritizes wrapping the arguments instead of the generic type definitions, resulting in more readable code:

Improved Output:

def func[T](
    a: T,
    b: T,
) -> T:
    return a

However, Black will still wrap the generic type declarations if:

  1. A magic trailing comma is present in the generic type declaration.
  2. The length of the line from the start of the function declaration to the closing square bracket approaches the line limit, making necessary wrapping it to conform to the limit.

For example:

Input:

def trailing_comma1[T=int,](a: str):
    pass

Output:

def trailing_comma1[
    T = int,
](a: str):
    pass

Implementation details:

In the left_hand_split function, black would search for an opening brackets leaf in function declaration. It was not differentiating between squared brackets and parenthesis, thus it was treating function parameters and generic type definitions the same way. Since generic type definitions comes first, it would always wrap them first.

The solution was to just make it ignore the opening of square brackets in all cases, expect:

  1. There was a magic trailing comma in the generic type definition.
  2. If the amount of characters from the start of the function declaration until the closing squared bracket would almost reach the line limit.

Checklist

  • Added an entry in CHANGES.md if necessary.
  • Added/updated tests to cover the changes.
  • Updated documentation as needed.

@JelleZijlstra
Copy link
Collaborator

This crashes with a comment on a line by itself after the typevar:

$ python -m black -c '''def with_random_comments[
>     Z
>     # bye
> ]():
>     return a'''
def with_random_comments[
    Z
    # bye
]():
    return a
$ python -m black -c '''def with_random_comments[
    Z
    # bye
]():
    return a''' --preview
def with_random_comments[
    Z
    # bye
]():
    return a
error: cannot format <string>: Cannot parse: 2:4:     return a

I'll push a few changes.

@JelleZijlstra
Copy link
Collaborator

The crash happens only if the comment is on a line by itself after the last type parameter. Black tries to reformat the code like this:

def with_random_comments[Z# bye]():
    return a

We should either leave the code alone or reformat it with the comment at the end of a collapsed line, like this:

$ python -m black -c '''def with_random_comments[
    Z    # bye
]():
    return a''' --fast --preview
def with_random_comments[Z]():  # bye
    return a

(I don't really care which of the two we pick, whatever is easiest to implement.)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, it's great!

I think the trailing comma handling isn't exactly right (but maybe this is fine), see e.g. both_magic in the new test cases I pushed. :-)

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 24, 2025

Okay, I have a fix for both things that I think also simplifies things a little, pushing...

@hauntsaninja
Copy link
Collaborator

@Pedro-Muller29 @JelleZijlstra please lmk what you both think!

@JelleZijlstra
Copy link
Collaborator

Nice, thanks for the fixes! I think we could use some more test cases with comments in random places, I might spend some time on that tomorrow.

@Pedro-Muller29
Copy link
Author

@Pedro-Muller29 @JelleZijlstra please lmk what you both think!

Nice changes!

It simplified the logic, and I think relying on bracket_split_build_line to determine whether the line should be split, instead of manually doing it as my changes were, is a better idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants