black icon indicating copy to clipboard operation
black copied to clipboard

Improve function definition wrapping

Open sumezulike opened this issue 11 months ago • 1 comments

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 ...

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

sumezulike avatar Mar 10 '24 22:03 sumezulike

diff-shades results comparing this PR (647dd2cd911b44f252de1683ebb658038a35ff54) to main (6af7d1109693c4ad3af08ecbc34649c232b47a6d). 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

github-actions[bot] avatar Mar 10 '24 23:03 github-actions[bot]

Why was this PR closed?

jbms avatar Jul 27 '24 23:07 jbms

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.

sumezulike avatar Jul 28 '24 00:07 sumezulike