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

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

Closed
alexreinking opened this issue Apr 15, 2022 · 9 comments · Fixed by #3916
Labels
F: trailing comma Full of magic T: bug Something isn't working

Comments

@alexreinking
Copy link

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 alexreinking added the T: bug Something isn't working label Apr 15, 2022
@JelleZijlstra JelleZijlstra added the F: trailing comma Full of magic label Apr 15, 2022
@KotlinIsland
Copy link
Contributor

or just:

a: list[int,]

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

@AIGeneratedUsername
Copy link

AIGeneratedUsername commented Oct 8, 2022

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.

@ichard26
Copy link
Collaborator

ichard26 commented Oct 8, 2022

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

@AIGeneratedUsername
Copy link

AIGeneratedUsername commented Oct 9, 2022

@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 #2316 (comment)

@jakkdl
Copy link
Contributor

jakkdl commented Sep 28, 2023

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:

# We should not remove the trailing comma in a single-element subscript.
a: tuple[int,]
b = tuple[int,]

@JelleZijlstra
Copy link
Collaborator

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

@jakkdl
Copy link
Contributor

jakkdl commented Sep 28, 2023

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 added a commit to jakkdl/black that referenced this issue Sep 29, 2023
@jakkdl
Copy link
Contributor

jakkdl commented Sep 29, 2023

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

@JelleZijlstra
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: trailing comma Full of magic T: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants