black icon indicating copy to clipboard operation
black copied to clipboard

Black collapses lists in return type annotations onto one line, regardless of the "magic" trailing comma.

Open alexreinking opened this issue 2 years ago • 4 comments

Describe the bug

Black collapses lists in return type annotations onto one line, regardless of the "magic" trailing comma.

To Reproduce

For example, take this code:

@hl.generator(name="simplepyfn")
def SimplePyFn(
    context: hl.GeneratorContext,
    buffer_input: Buffer[UInt8, 2],
    func_input: Buffer[Int32, 2],
    float_arg: Scalar[Float32],
    offset: int = 0,
) -> tuple[
    Buffer[UInt8, 2],
    Buffer[UInt8, 2],
]:
    """
    ... more code here ...
    """

And run it with these arguments:

$ black file.py --target-version py39

The resulting file contains:

@hl.generator(name="simplepyfn")
def SimplePyFn(
    context: hl.GeneratorContext,
    buffer_input: Buffer[UInt8, 2],
    func_input: Buffer[Int32, 2],
    float_arg: Scalar[Float32],
    offset: int = 0,
) -> tuple[Buffer[UInt8, 2], Buffer[UInt8, 2],]:
    """
    ... more code here ...
    """

Expected behavior

I would have expected the code to either

  1. Not be reformatted due to the magic trailing comma (preferred)
  2. Have the trailing comma removed. (worse)

Environment

  • Black's version: black, 22.3.0 (compiled: yes)
  • OS and Python version: Ubuntu 20.04.3 LTS (WSL), Python 3.8.10

alexreinking avatar Apr 15 '22 19:04 alexreinking

or just:

a: list[int,]

With skip-magic-trailing-comma nothing is changed.

KotlinIsland avatar Jul 21 '22 06:07 KotlinIsland

I've come here searching for the opposite problem. Black keeps splitting my type annotations (e.g. value: list[FooBar]) into multiple lines. It is a very bad practice because functions with multi-line type annotations are unreadable. I hope that Black will stop touching type annotations... Or at least will not touch type annotations that have no trailing comma.

simon-liebehenschel avatar Oct 08 '22 16:10 simon-liebehenschel

@AIGeneratedUsername are you talking about our subpar formatting of the new union syntax (see https://github.com/psf/black/issues/2316) or the formatting of type annotations in general? I don't think it's tenable to just leave type annotations alone.

ichard26 avatar Oct 08 '22 23:10 ichard26

@AIGeneratedUsername are you talking about our subpar formatting of the new union syntax (see #2316) or the formatting of type annotations in general? I don't think it's tenable to just leave type annotations alone.

Thank you for pointing to a related issue. I've found that my complain while not fully, but mostly described by https://github.com/psf/black/issues/2316#issuecomment-1007878234

simon-liebehenschel avatar Oct 09 '22 18:10 simon-liebehenschel

or just:

a: list[int,]

With skip-magic-trailing-comma nothing is changed.

This is intended behavior (though I'm not sure why), and probably unrelated: https://github.com/psf/black/blob/3dcacdda0d7f69a1705f3e2a151c24a6cf004171/tests/data/simple_cases/skip_magic_trailing_comma.py#L1-L3

jakkdl avatar Sep 28 '23 12:09 jakkdl

@jakkdl We shouldn't remove the trailing comma in that case because it changes the AST.

JelleZijlstra avatar Sep 28 '23 13:09 JelleZijlstra

I dug further into this, and it ended up revealing that left_hand_split() needs its logic revamped since the addition of return types. I found a bunch of other incorrect cases:

Long return type causes parameter list to get split unnecessarily

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

magic trailing comma in return type causes param list to get split

def foo(a, b) -> list[a, a,]: ...
# output
def foo(
    a, b
) -> list[a, a,]: ...

replacing square brackets with parens gives same behaviour.

So there may be other open issues that would get resolved by the same fix. I should be able to write a PR soonish now that I've nailed down where the problem is.

jakkdl avatar Sep 28 '23 15:09 jakkdl

@JelleZijlstra (and/or others) I got one case which I'm a bit iffy about, currently the code will reformat very long single-name return types and function names like so:

def foo() -> (
    intsdfsafafafdfdsasdfsfsdfasdfafdsafdfdsfasdskdsdsfdsafdsafsdfdasfffsfdsfdsafafhdskfhdsfjdslkfdlfsdkjhsdfjkdshfkljds
):
    return 2

def thiiiiiiiiiiiiiiiiiis_iiiiiiiiiiiiiiiiiiiiiiiiiiiiiis_veeeeeeeeeeeeeeeeeeeeeeery_looooooong() -> (
    list[int, float]
): ...

but I'm partial to this variant instead, which I especially prefer when there's a return type:

def foo(
) -> intsdfsafafafdfdsasdfsfsdfasdfafdsafdfdsfasdskdsdsfdsafdsafsdfdasfffsfdsfdsafafhdskfhdsfjdslkfdlfsdkjhsdfjkdshfkljds:
    return 2

def thiiiiiiiiiiiiiiiiiis_iiiiiiiiiiiiiiiiiiiiiiiiiiiiiis_veeeeeeeeeeeeeeeeeeeeeeery_looooooong(
) -> list[int, float]: ...

I can make the code follow the former and minimize changes no problem, or are we fine with changing to the latter? Admittedly these are very obscure cases so it probably doesn't matter a ton either way, and as such should maybe just minimize any changes.

jakkdl avatar Sep 29 '23 15:09 jakkdl

I think I prefer the former, it looks weird to have the parentheses on separate lines with nothing in between.

JelleZijlstra avatar Sep 29 '23 15:09 JelleZijlstra