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

magic trailing comma not working in certain scenario #3239

Closed
nipunn1313 opened this issue Aug 26, 2022 · 9 comments
Closed

magic trailing comma not working in certain scenario #3239

nipunn1313 opened this issue Aug 26, 2022 · 9 comments
Labels
F: trailing comma Full of magic S: awaiting response Waiting for futher information from OP T: bug Something isn't working

Comments

@nipunn1313
Copy link
Contributor

Describe the bug

Trailing commas are incorrectly being ruthlessly collapsed into one line

To Reproduce

Reproes in the black playground

def method(
    self,
) -> typing_extensions.Literal[
    "l1",
    "l2",
]: pass

Result is

def method(
    self,
) -> typing_extensions.Literal["l1", "l2",]:
    pass

Expected behavior

I'd expect no change to the Literal writeout.

Environment

Black 22.6.0 in the black playground.

Additional context

I noticed that if we eliminate the self argument, then black works correctly. Seems to have something to do with the fact that there are two trailing command in the method definition. Second one is not respected.

@nipunn1313 nipunn1313 added the T: bug Something isn't working label Aug 26, 2022
@ichard26 ichard26 added the F: trailing comma Full of magic label Aug 26, 2022
@hauntsaninja
Copy link
Collaborator

I believe I've fixed this on main (you'll need to remember to use --preview because it's a change that changed formatting)

@ichard26 ichard26 added the S: awaiting response Waiting for futher information from OP label Aug 27, 2022
@JelleZijlstra
Copy link
Collaborator

Yes, not fixed yet:

$ black --diff --preview tryit.py 
--- tryit.py    2022-09-02 02:02:08.120028 +0000
+++ tryit.py    2022-09-02 02:02:16.343920 +0000
@@ -1,6 +1,4 @@
 def method(
     self,
-) -> typing_extensions.Literal[
-    "l1",
-    "l2",
-]: pass
+) -> typing_extensions.Literal["l1", "l2",]:
+    pass
would reformat tryit.py

All done! ✨ 🍰 ✨
1 file would be reformatted.
$ black --version
black, 22.8.0 (compiled: yes)
Python (CPython) 3.9.12

(22.8.0 is the same as main right now)

@hauntsaninja
Copy link
Collaborator

@nipunn1313 your playground link does not check the box that says "Don't use trailing commas as a reason to split lines."
@JelleZijlstra you didn't pass --skip-magic-trailing-comma

~/dev/black main λ black --diff --preview x.py -C
--- x.py	2022-09-02 02:06:01.742586 +0000
+++ x.py	2022-09-02 02:07:53.238628 +0000
@@ -1,6 +1,2 @@
-def method(
-    self,
-) -> typing_extensions.Literal[
-    "l1",
-    "l2",
-]: pass
+def method(self) -> typing_extensions.Literal["l1", "l2"]:
+    pass
would reformat x.py

All done! ✨ 🍰 ✨
1 file would be reformatted.

@nipunn1313
Copy link
Contributor Author

I want the default behavior of the magic trailing comma!

It appears that the magic trailing comma's behavior is not being respected in this case.

@hauntsaninja
Copy link
Collaborator

Okay, sure. Your issue title skip_magic_trailing_comma=true seems incorrect then?

Note that this was a trailing comma that wasn't removed with --skip-magic-trailing-comma until #3209, hence my claim in #3239 (comment)

@nipunn1313
Copy link
Contributor Author

OOPS. I totally inverted my statement in the title of the task! Booleans are hard.
Editing title

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 2, 2022

I think your example is basically a duplicate of #2498 which was closed as a duplicate of #1671 although the issue title of #1671 doesn't quite apply to your case / #2498

@nipunn1313 nipunn1313 changed the title skip_magic_trailing_comma=true not working in certain scenario magic trailing comma not working in certain scenario Sep 2, 2022
@JelleZijlstra
Copy link
Collaborator

Yeah I think this is basically #1671.

@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2022
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 S: awaiting response Waiting for futher information from OP T: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants