Unexpected change to imports when updating to isort 6.0.0

[sarahboyce]

On Django, our isort config is:

[tool.isort]
profile = "black"
default_section = "THIRDPARTY"
known_first_party = "django"

With isort==5.13.2, the following imports are sorted as:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import SQLUpdateCompiler

But on isort==6.0.0, they are now sorted as:

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
from django.db.models.sql.compiler import (
    SQLUpdateCompiler,
)

We don't believe this is a desirable change and so are pinned at 5.13.2

(Please let me know if you need more details. Our pr for reference: django/django#19109)

[LepkoQQ]

We also noticed this issue, but with an extra step. This doesn't always happen the first time depending on the input.

For example if you start with code like this:

from another_file_aaaaaaaaaaaaaaaaaa import \
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func

then you run isort . --profile black and the output is:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import some_func

then you run isort . --profile black AGAIN and then the output is:

from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import (
    some_func,
)

[aholmes]

This seems related to this breaking change (not documented as such!) in default behavior. #2236

In my case it breaks compatibility with ruff, but the result is exactly the same. Here is the relevant Dependabot PR (with the fix) in my affected repository. https://github.com/uclahs-cds/Ligare/pull/175/files

diff --git a/pyproject.toml b/pyproject.toml
index c3dd914..2a98b33 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -284,8 +284,11 @@ skip-magic-trailing-comma = false
 line-ending = "auto"
 docstring-code-format = false
 docstring-code-line-length = "dynamic"
+[tool.ruff.lint.isort]
+split-on-trailing-comma = false

 [tool.isort]
 profile = "black"
 src_paths = ["src"]
 skip_glob = ["src/*/typings", "src/*/build"]
+split_on_trailing_comma = false

[staticdev]

@aholmes technically it is not a breaking anything. It is just a regular improvement on sorting. Django is a projecto se have integration tests for QA, but we had to comment because of this single case.

@sarahboyce I would advice to upgrade to 6.0.0 and keep the new sorting. It will work perfectly and everything is sorted as it should.

[aholmes]

@staticdev correct me if I'm mistaken, but this is an observable change in isort's default behavior, right?

[LepkoQQ]

Even if this is an intended change, (which I think is still not correct since the second import did not have a trailing comma that would make it split lines) - I can live with that if you think it is correct.

However the problem I described in my comment is still an issue in my opinion since it requires running isort twice to stabilize the sort. It already happened to us that a developer ran isort locally only once and pushed the changes which then failed on CI because the output changes when you run isort the second time.

Initially I thought this was related, but if the change is intended I can split the "you have to run isort twice" in to to a separate issue if you like.

[staticdev]

@LepkoQQ in this case I would definitely agree. Isort must be stable. I would like to confirm it is really happening.

[DanielNoord]

I would agree that this is indeed a regression, even if the sorting is stable.

I hope to devote some time tonight to look into this.

I must confess when I looked at the original django diff I wasn't 99% sure it was correct but since the PR fixed a clear bug I leaned towards "It is likely correct". Considering users are now raising this as a bug that was likely the incorrect opinion. Sorry about that!

[staticdev]

@LepkoQQ I just cloned the Django repository and actually the sorting is stable, it works on the first time (no additional changes on second run).
If @LepkoQQ and @DanielNoord agree I will close this issue as not a bug, this is a feature to me.

[sarahboyce]

The Django repo already had isort applied at least once. I can replicate with the instructions of this comment 👍

[aholmes]

@staticdev Thanks for confirming. I think I should clarify my usage of "breaking change." I don't mean to say that this change is broken or wrong; I mean that the change alters previous behavior and, as a result, potentially creates downstream breakages. In my case, this change causes my CI pipeline to fail because isort's expectation does not match ruff's formatting. This is important to communicate in the release notes. Because it was not indicated there, I (and maybe others) had to do a little digging into PRs to discover what had changed.

Either way, the point is somewhat moot because this is in a major-version change, anyway. I appreciate the work that went into this release and to improve isort; I just wish this observable change in behavior was reported in the release notes and wanted to highlight this point for future consideration.

[aholmes]

By the way I can also replicate #2352 (comment)

#!/bin/bash
read -r -d '' UNFORMATTED <<EOF
from another_file_aaaaaaaaaaaaaaaaaa import \\
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func
EOF

echo Unformatted:
echo "$UNFORMATTED"
echo '-------------------------------------------------'

FORMATTED1="`isort --profile black - <<<"$UNFORMATTED"`"
echo Formatted 1:
echo "$FORMATTED1"
echo '-------------------------------------------------'

FORMATTED2="`isort --profile black - <<<"$FORMATTED1"`"
echo Formatted 2:
echo "$FORMATTED2"

Unformatted:
from another_file_aaaaaaaaaaaaaaaaaa import \
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED
from another_file_aaaaaaaaaaaaaaaaaa import some_func
-------------------------------------------------
Formatted 1:
from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import some_func
-------------------------------------------------
Formatted 2:
from another_file_aaaaaaaaaaaaaaaaaa import (
    SOME_THING_CONSTANT as SOME_THING_CONSTANT_RENAMED,
)
from another_file_aaaaaaaaaaaaaaaaaa import (
    some_func,
)

[DanielNoord]

@sarahboyce What would the desired output be of the original code block?

Wouldn't this be more correct?

from django.db.models.sql.compiler import (
    SQLAggregateCompiler,
    SQLCompiler,
    SQLDeleteCompiler,
    SQLUpdateCompiler,
)
from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler

What about the black profile makes is so that SQLUpdateCompiler should remain on its own line? Perhaps @staticdev knows more about this.

Anyway, this is indeed a regression. What (I think) is happening is that we "correctly" identify that django.db.models.sql.compiler is using a trailing comma, but then we also apply that to the line with SQLUpdateCompiler and add a comma there. Even though it is below the max line length of the profile.

If @staticdev gives me some pointers I can likely try and come up with a fix that either keeps the code as is, or puts the import of SQLUpdateCompiler with the other imports that don't use as.

[DanielNoord]

A fix with test would be:

diff --git a/isort/output.py b/isort/output.py
index ade4ad29..e098d858 100644
--- a/isort/output.py
+++ b/isort/output.py
@@ -513,7 +513,7 @@ def _with_from_imports(
                     do_multiline_reformat = True
 
                 if (
-                    import_statement
+                    len(import_statement) > config.line_length
                     and config.split_on_trailing_comma
                     and module in parsed.trailing_commas
                 ):
diff --git a/tests/integration/test_projects_using_isort.py b/tests/integration/test_projects_using_isort.py
index 61f66e7e..01ce9835 100644
--- a/tests/integration/test_projects_using_isort.py
+++ b/tests/integration/test_projects_using_isort.py
@@ -28,10 +28,6 @@ def run_isort(arguments: Generator[str, None, None] | Sequence[str]):
     main(["--check-only", "--diff", *arguments])
 
 
-@pytest.mark.xfail(
-    reason="Project is incorrectly formatted after PR #2236, should be fixed "
-    "after a release and the project formatting again."
-)
 def test_django(tmpdir):
     git_clone("https://github.com/django/django.git", tmpdir)
     run_isort(
diff --git a/tests/unit/profiles/test_black.py b/tests/unit/profiles/test_black.py
index 209d83d8..19885d77 100644
--- a/tests/unit/profiles/test_black.py
+++ b/tests/unit/profiles/test_black.py
@@ -456,3 +456,24 @@ from x import (
 )
 """,
     )
+
+
+def test_black_trailing_comma_below_line_length():
+    black_test(
+"""from django.db.models.sql.compiler import (
+    SQLAggregateCompiler,
+    SQLCompiler,
+    SQLDeleteCompiler,
+)
+from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
+from django.db.models.sql.compiler import SQLUpdateCompiler
+""",
+"""from django.db.models.sql.compiler import (
+    SQLAggregateCompiler,
+    SQLCompiler,
+    SQLDeleteCompiler,
+)
+from django.db.models.sql.compiler import SQLInsertCompiler as BaseSQLInsertCompiler
+from django.db.models.sql.compiler import SQLUpdateCompiler
+""",
+    )

However, this doesn't actually work as we would no longer respect actual trailing commas if the line length is below the max line length. The issue is that at the place where I make the change to len(import_statement) > config.line_length we don't seem to have access to the original line to check whether it has a trailing comma.

This was originally implemented with module in parsed.trailing_commas is True, but in this case it actually shouldn't be True. django.db.models.sql.compiler does indeed have a trailing comma, but not all the time. The current implementation can't account for this and I don't see an easy way to allow for this.