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 definition wrapping #4272

Closed

Conversation

sumezulike
Copy link
Contributor

Description

This is intended to fix issues like #3929, #4071, and #4254, where the addition of the new type parameter syntax in Python 3.12 has caused suboptimal formatting of function definitions.

The new transformer for function definition inspects the given function definition, then splits at the first thing found from the following list:

  • function parameters with magic trailing comma
  • return type with magic trailing comma
  • type parameters with magic trailing comma
  • return type longer than max line length
  • type parameters longer than max line length
  • function parameters (as default)

For example, assuming a very small max line length

def f[T](a): ...

is formatted as

def f[T](
    a
): ...

but

def f[T,](a): ...

becomes

def f[
    T,
](a): ...

because the trailing comma gives the type parameters a higher priority.

This only really changes how functions with type parameters are split compared to the stable style.

A function that only has one of type params, function params or a return type is just passed to left_hand_split.

Possible issues

One test is currently failing, in funcdef_return_type_trailing_comma.py:

def foo(a) -> list[
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
]:  # abpedeifnore
    pass
# output
def foo(
    a,
) -> list[
    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
]:  # abpedeifnore
    pass

With the new formatter, the test input is kept as is, which I feel is better.

I added the part with the long strings because it seems logical and some comments were considering going in that direction.
line_to_string has a warning about being computationally expensive, but I would think it should be fine for two lines.
A longer character width than the max line length might not be the most useful check though.

If the choice for splitting falls on return type, the line is passed to right_hand_split. That was my solution to not duplicate any more code, but that could maybe introduce inconsistencies? I couldn't think of any, but it feels a bit off.

I'm grateful for any feedback and suggestions!

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Splits function definitions while respecting trailing commas and prioritising parts that are longer than line length.
@sumezulike sumezulike changed the title Improve type param function wrapping Improve function definition wrapping Mar 10, 2024
Copy link

diff-shades results comparing this PR (647dd2c) to main (6af7d11). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 3 projects & 4 files changed / 16 changes [+4/-12]     │
│                                                        │
│ ... out of 2 545 577 lines, 12 286 files & 22 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@sumezulike sumezulike closed this Jun 7, 2024
@jbms
Copy link

jbms commented Jul 27, 2024

Why was this PR closed?

@sumezulike
Copy link
Contributor Author

I closed it because this issue turned out to require more extensive and complex changes than I initially assumed. Since I sadly didn't have the time to figure out all the intricacies in blacks inner workings I had to give it up. You're welcome to continue where I left off although I'm not sure I was on the right path.

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.

2 participants