astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
30.95k stars 1.02k forks source link

`# isort: split` ignored if in trailing comment #11776

Open ndevenish opened 3 months ago

ndevenish commented 3 months ago

test.py:

import x
import y

import f # isort: split

import a
import b

isort is a noop:

% isort --version
                 _                 _
                (_) ___  ___  _ __| |_
                | |/ _/ / _ \/ '__  _/
                | |\__ \/\_\/| |  | |_
                |_|\___/\___/\_/   \_/

      isort your imports, so you don't have to.

                    VERSION 5.13.2
% isort --diff test.py
%

ruff ignores the marker:

% ruff --version
ruff 0.4.8
% ruff check --select=I --diff test.py
--- test.py
+++ test.py
@@ -1,8 +1,7 @@

-import x
-import y
-
-import f # isort: split
+import f  # isort: split

 import a
 import b
+import x
+import y

Would fix 3 errors.
charliermarsh commented 3 months ago

I find it a bit confusing. How should the import f be treated in such a case? Is it "before" or "after" the split, or neither?

charliermarsh commented 3 months ago

I'm tempted to call this unsupported. Do you have a use-case for the inline split?

ndevenish commented 3 months ago

I find it a bit confusing. How should the import f be treated in such a case? Is it "before" or "after" the split, or neither?

It's effectively treated as

import x
import y

# isort: split
import f
# isort: split

import a
import b

This is explicitly documented in isort: https://pycqa.github.io/isort/docs/configuration/action_comments.html#isort-split

You can also use it inline to keep an import from having imports above or below it swap position:


I'm tempted to call this unsupported. Do you have a use-case for the inline split?

We've used this in the past for cases where upstream dependencies have not handled arbitrary-import-order very well, and so a specific import needs to be "First". We can work around by using normal split/on/off features.

Perhaps the fact that we're the only ones to notice this in so long is perhaps evidence that supporting it isn't very important.

seproDev commented 3 months ago

I am not sure if this is 100% related, but we also ran in to differences with trailing # isort: split. This is a noop in isort 5.13.2:

from x import a  # isort: split
from x import b

While ruff 0.4.8 changes it to:

from x import a  # isort: split

from x import b

When running

ruff check --select I --fix --isolated
charliermarsh commented 3 months ago

Yeah I think we just don't really support inline # isort: split right now.

XuehaiPan commented 1 month ago

From the isort documentation: https://pycqa.github.io/isort/docs/configuration/action_comments.html#isort-split

isort split is exactly the same as placing an # isort: on immediately below an # isort: off

Originally posted at https://github.com/pytorch/pytorch/pull/132574#discussion_r1703287249:

ruff check --select=I --fix do not respect # isort: split for asterisk imports. This is a bug.

from xxx.b import *  # isort: split
from xxx.a import *  # isort: split  # avoid circular imports

will be format to:

from xxx.a import *  # isort: split  # avoid circular imports
from xxx.b import *  # isort: split

The # isort: split inline comment should be equivalent to:

# isort: off
from xxx.b import *
# isort: on
# isort: off
from xxx.a import *  # avoid circular imports
# isort: on