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

isort inserts a trailing comma even if there's just a single name in parentheses #250

Closed
mxr opened this issue May 22, 2018 · 6 comments
Closed
Labels
F: trailing comma Full of magic T: enhancement New feature or request

Comments

@mxr
Copy link

mxr commented May 22, 2018

Operating system: Mac OS 10.13.4
Python version: 3.6.5 (through pyenv)
Black version: 18.4a4
Does also happen on master: yes


Given this setup:

$ tail -n+1 foo.py .pre-commit-config.yaml setup.cfg
==> foo.py <==
from a_really.really.really.really.really.long.package.namespace import a_really_long_package
from really.really.really.really.really.long.package.namespace import really_long_package as pkg

==> .pre-commit-config.yaml <==
repos:
- repo: https://github.com/pre-commit/mirrors-isort
  rev: v4.3.4
  hooks:
  - id: isort
- repo: https://github.com/ambv/black
  rev: 18.5b0
  hooks:
  - id: black
    python_version: python3.6

==> setup.cfg <==
[isort]
# Using config from
# https://github.com/ambv/black/blame/c7bc22388d30f1ba503eefd574e4bb794749b782/README.md
force_grid_wrap = 0
include_trailing_comma = true
line_length = 88
multi_line_output = 3

Running isort changes the file to

from a_really.really.really.really.really.long.package.namespace import (
    a_really_long_package,
)
from really.really.really.really.really.long.package.namespace import \
    really_long_package as pkg

Running black changes the file to

from a_really.really.really.really.really.long.package.namespace import (
    a_really_long_package
)
from really.really.really.really.really.long.package.namespace import (
    really_long_package as pkg
)

The difference being the trailing comma and the \ line break

--- foo_isort.py	2018-05-22 15:46:53.000000000 -0400
+++ foo_black.py	2018-05-22 15:46:46.000000000 -0400
@@ -1,5 +1,6 @@
 from a_really.really.really.really.really.long.package.namespace import (
-    a_really_long_package,
+    a_really_long_package
 )
-from really.really.really.really.really.long.package.namespace import \
+from really.really.really.really.really.long.package.namespace import (
     really_long_package as pkg
+)

If I add this option to setup.cfg

--- setup.cfg	2018-05-22 15:39:19.000000000 -0400
+++ setup.cfg.new	2018-05-22 15:47:41.000000000 -0400
@@ -1,6 +1,5 @@
 [isort]
-# Using config from
-# https://github.com/ambv/black/blame/c7bc22388d30f1ba503eefd574e4bb794749b782/README.md
+combine_as_imports = true
 force_grid_wrap = 0
 include_trailing_comma = true
 line_length = 88

I get a slightly closer diff, but the trailing commas are inconsistent.

--- foo_isort.py	2018-05-22 15:48:59.000000000 -0400
+++ foo_black.py	2018-05-22 15:49:04.000000000 -0400
@@ -1,6 +1,6 @@
 from a_really.really.really.really.really.long.package.namespace import (
-    a_really_long_package,
+    a_really_long_package
 )
 from really.really.really.really.really.long.package.namespace import (
-    really_long_package as pkg,
+    really_long_package as pkg
 )
@ambv
Copy link
Collaborator

ambv commented May 23, 2018

I will add combine_as_imports to our recommendation, thanks for the find.

I will fix the missing trailing comma when there is a single import that doesn't fit in a line, too.

@mxr
Copy link
Author

mxr commented May 23, 2018

Thanks!

@ambv ambv changed the title isort==4.3.4 incompatible with black==18.5b0 isort inserts a trailing comma even if there's just a single name in parentheses May 29, 2018
@ambv ambv added the T: enhancement New feature or request label May 29, 2018
@ambv ambv added the F: trailing comma Full of magic label Jun 20, 2018
@jacebrowning
Copy link

We're still running into this issue and looking forward to a fix. 😄

@mxr
Copy link
Author

mxr commented Sep 6, 2018

I switched my affected repos to a different import sorting tool (reorder_python_imports) to work around this, not to diminish the hard work of the black maintainers in working on a fix!

@tony
Copy link
Contributor

tony commented Sep 7, 2018

@ambv thanks for taking a look at this! and thanks for black!

At the moment, I'm just rerunning isort implicitly after black runs. I think (hopefully) this is the last thing to get isort/black playing together nicely.

@cas--
Copy link
Contributor

cas-- commented Oct 2, 2018

Please replace combine_as_imports=True as that is forcing an unwanted isort style as a side effect of fixing this issue. The actual setting should be use_parentheses=True

cas-- added a commit to cas--/black that referenced this issue Oct 2, 2018
The `combine_as_imports=True` modifies isort style as a side-effect and was not the intended purpose of the suggested change in psf#250. The problem was that isort was actually replacing the parens with backslash and using `combine_as_imports=True` happened to also produce the same result.

The actual setting should be `use_parentheses` as this tells isort to use parenthesis for line continuation instead of \ for lines over the allotted line length limit and matches precisely what black is outputting.
cas-- added a commit to cas--/black that referenced this issue Oct 2, 2018
The `combine_as_imports=True` modifies isort style as a side-effect and was not the intended purpose of the suggested change in psf#250. The problem was that isort was actually replacing the parens with backslash and using `combine_as_imports=True` happened to also produce the same result.

The actual setting should be `use_parentheses` as this tells isort to use parenthesis for line continuation instead of \ for lines over the allotted line length limit and matches precisely what black is outputting.
ambv pushed a commit that referenced this issue Nov 8, 2018
…547)

The `combine_as_imports=True` modifies isort style as a side-effect and was not the intended purpose of the suggested change in #250. The problem was that isort was actually replacing the parens with backslash and using `combine_as_imports=True` happened to also produce the same result.

The actual setting should be `use_parentheses` as this tells isort to use parenthesis for line continuation instead of \ for lines over the allotted line length limit and matches precisely what black is outputting.
jleclanche pushed a commit to jleclanche/tan that referenced this issue Nov 14, 2018
jleclanche pushed a commit to jleclanche/tan that referenced this issue Nov 14, 2018
…sf#547)

The `combine_as_imports=True` modifies isort style as a side-effect and was not the intended purpose of the suggested change in psf#250. The problem was that isort was actually replacing the parens with backslash and using `combine_as_imports=True` happened to also produce the same result.

The actual setting should be `use_parentheses` as this tells isort to use parenthesis for line continuation instead of \ for lines over the allotted line length limit and matches precisely what black is outputting.

(cherry picked from commit 158f796)
davidbgk added a commit to betagouv/zam that referenced this issue Aug 14, 2019
black requires to be updated too because of psf/black#250
davidbgk added a commit to betagouv/zam that referenced this issue Aug 19, 2019
black requires to be updated too because of psf/black#250
davidbgk added a commit to betagouv/zam that referenced this issue Aug 19, 2019
black requires to be updated too because of psf/black#250
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: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants