Potential inconsistency between Ruff's ``I`` and isort

[AA-Turner]

I believe this is distinct from https://github.com/charliermarsh/ruff/issues/1718; cross-linking in case there are similarities, though.

isort version: 5.11.4 Ruff version: 230

isort config:

[tool.isort]
line_length = 95
profile = "black"
remove_redundant_aliases = true

Commands:

(sphinx) PS I:\Development\sphinx> ruff -V        
ruff 0.0.230
(sphinx) PS I:\Development\sphinx> type bug.py                                   
from operator import add  # NoQA: F401
from operator import mul, sub
(sphinx) PS I:\Development\sphinx> ruff --isolated --select I bug.py
bug.py:1:1: I001 Import block is un-sorted or un-formatted
Found 1 error(s).
1 potentially fixable with the --fix option.
(sphinx) PS I:\Development\sphinx> isort --check bug.py 

Note Ruff wants to re-format to:

from operator import (
    add,  # NoQA: F401
    mul,
    sub,
)

whereas isort wants to reformat to:

from operator import add  # NoQA: F401
from operator import mul, sub

This appeared in the Sphinx project where some imports are type-comment only (e.g. in giving type annotations to iterators, where the pattern is for item in items: # type: blah`, should MyPy not be able to deduce the type of item``).

A

[charliermarsh]

This does look like an incompatibility. I think the thing to decide is whether it's a known and accepted deviation, or not :)

Do you have a preference between the outputs? Did it cause any problems, or the issue is more that it deviated at all?

[Jackenmen]

Personally, I like Ruff's variant more as it keeps imports from a specific module in one statement. I don't know if the noqa comment applies properly in both cases though.

[charliermarsh]

Ruff will respect that noqa in both cases, but I don't think Flake8 respects the version that Ruff outputs.

[AA-Turner]

the issue is more that it deviated at all?

This is the case for me, we run both isort and Ruff in continuous integration testing so either will fail unless an ungainly isort: skip or ruff-specific NoQA comment were added.

I don't particularly mind which style 'wins'.

A

[skshetry]

I prefer isort's output here as it is more granular for the ignores/disables to work. pylint for example cannot work with inlined-disables. Take this code for example, from a codebase that I work with:

from dvc.api.data import open  # pylint: disable=redefined-builtin
from dvc.api.data import read

__all__ = ["read", "open"]

ruff changes it to:

--- script.py
+++ script.py
@@ -1,4 +1,6 @@
-from dvc.api.data import open  # pylint: disable=redefined-builtin
-from dvc.api.data import read
+from dvc.api.data import (
+    open,  # pylint: disable=redefined-builtin
+    read,
+)
 
 __all__ = ["read", "open"]

And, pylint now fails:

script.py:1:0: W0622: Redefining built-in 'open' (redefined-builtin)

[charliermarsh]

Yeah I think we should likely change this going forward given the handling of action comments.

[andersk]

pylint for example cannot work with inlined-disables.

Sure it can, you just need to move the comment to the right line.

from dvc.api.data import (  # pylint: disable=redefined-builtin
    open,
    read,
)

__all__ = ["read", "open"]

Ruff can’t be sure in advance which line the comment belongs on (and honestly, its first guess makes more sense), but once you put it on the right line, Ruff will leave it there.

[charliermarsh]

I'm going to close this for now as an intentional deviation. If we receive more reports or more feedback on it, as always, I'll reconsider.