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 23.1 no longer respects trailing magic comma for single length list #3542

Closed
gaborbernat opened this issue Feb 2, 2023 · 6 comments
Closed
Labels
F: trailing comma Full of magic R: not a bug This is deliberate behavior of Black. T: bug Something isn't working T: style What do we want Blackened code to look like?

Comments

@gaborbernat
Copy link
Contributor

gaborbernat commented Feb 2, 2023

from typing import Literal

A = Literal[
    "DEFAULT",
]
❯ black magic.py --check
would reformat magic.py

Oh no! πŸ’₯ πŸ’” πŸ’₯
1 file would be reformatted.

It turns it into:

from typing import Literal

A = Literal["DEFAULT",]

I personally would prefer to respect trailing magic commas here, but at the very least remove the trailing comma if is going to be inlined into a single line.

@gaborbernat gaborbernat added the T: bug Something isn't working label Feb 2, 2023
@ichard26 ichard26 added T: style What do we want Blackened code to look like? R: not a bug This is deliberate behavior of Black. F: trailing comma Full of magic labels Feb 3, 2023
@ichard26
Copy link
Collaborator

ichard26 commented Feb 3, 2023

This is as intended. See https://ichard26.github.io/blog/2022/12/black-23.1a1/#ignore-magic-trailing-comma-for-single-element-subscripts and #2918.

This probably isn't documented in the style documentation though. Definitely important to fix that.

@felix-hilden
Copy link
Collaborator

Definite +1 to removing the trailing comma on a single line though πŸ˜„

@JelleZijlstra
Copy link
Collaborator

Definite +1 to removing the trailing comma on a single line though πŸ˜„

That would be an AST-unsafe change. It happens to be safe for types, but it isn't safe in general.

In [30]: print(ast.dump(ast.parse("a[1]"), indent=2))
Module(
  body=[
    Expr(
      value=Subscript(
        value=Name(id='a', ctx=Load()),
        slice=Constant(value=1),
        ctx=Load()))],
  type_ignores=[])

In [31]: print(ast.dump(ast.parse("a[1,]"), indent=2))
Module(
  body=[
    Expr(
      value=Subscript(
        value=Name(id='a', ctx=Load()),
        slice=Tuple(
          elts=[
            Constant(value=1)],
          ctx=Load()),
        ctx=Load()))],
  type_ignores=[])

@felix-hilden
Copy link
Collaborator

Oh true πŸ‘

@hauntsaninja
Copy link
Collaborator

The only alternative I see here is something like A = Literal[("DEFAULT",)] ... thoughts? Otherwise, we should just close this issue, because Black shouldn't modify AST.

@hauntsaninja
Copy link
Collaborator

Closing as per my comment above (since no one seems to have loved the parenthesised tuple suggestion)

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 R: not a bug This is deliberate behavior of Black. T: bug Something isn't working T: style What do we want Blackened code to look like?
Projects
None yet
Development

No branches or pull requests

5 participants