Skip to content

fix: keep comment-only lines inside from-import groups - #2588

Open
alexchen-sys wants to merge 2 commits into
PyCQA:mainfrom
alexchen-sys:issue/1852-preserve-comment-only-import-lines
Open

fix: keep comment-only lines inside from-import groups#2588
alexchen-sys wants to merge 2 commits into
PyCQA:mainfrom
alexchen-sys:issue/1852-preserve-comment-only-import-lines

Conversation

@alexchen-sys

Copy link
Copy Markdown

Summary

Comment-only lines inside a parenthesised from … import ( group were collapsed onto the opening import line as one semicolon-joined comment. With the black profile that rewrote intentional WIP comments and produced lines that fail line-length checkers.

This change tracks those body comments separately. When parentheses are used, they are re-emitted as their own indented comment lines inside the group. Non-parenthesised wrap modes keep the historical collapse behaviour so existing cases such as issue 1396 stay stable.

Test plan

  • pytest tests/unit/test_regressions.py::test_comment_only_lines_in_from_import_group_issue_1852
  • pytest tests/unit/test_regressions.py::test_incorrect_grouping_when_comments_issue_1396
  • pytest tests/unit/test_regressions.py::test_comment_on_opening_line_of_aliased_import_does_not_move
  • pytest tests/unit/test_isort.py tests/unit/test_regressions.py tests/unit/test_comments.py (331 passed, 1 skipped)

Fixes #1852

@alexchen-sys

Copy link
Copy Markdown
Author

DeepSource was failing because the existing skipcq: PY-R1000 comment landed on the small helper I added instead of _with_from_imports.

Moved the skip back onto _with_from_imports (same pattern as before). No product change; regression for #1852 still passes.

@staticdev
staticdev force-pushed the issue/1852-preserve-comment-only-import-lines branch from ee1bc23 to 468d670 Compare July 18, 2026 15:04
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.37%. Comparing base (d5f40fa) to head (247f001).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2588   +/-   ##
=======================================
  Coverage   99.36%   99.37%           
=======================================
  Files          41       41           
  Lines        3165     3212   +47     
  Branches      682      696   +14     
=======================================
+ Hits         3145     3192   +47     
  Misses         12       12           
  Partials        8        8           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alexchen-sys

Copy link
Copy Markdown
Author

Fixed flake8 E303 by removing one extra blank line before the #1852 regression test. No product change.

@alexchen-sys

Copy link
Copy Markdown
Author

Lint Attempt #2 failed on ruff format --check for tests/unit/test_regressions.py (not flake8 E303). Applied ruff format to the #1852 regression test only; product behavior unchanged. Focused pytest still passes.

@alexchen-sys

Copy link
Copy Markdown
Author

Residual loop closeout (multi-stage A/C/D/B1/B2 + mechanical gate PASS):

No further product thrash planned; remaining secondary nits (as-only / all-comment-only) disclosed as ALLOW_WITH_NITS scope.

alexchen-sys added a commit to alexchen-sys/isort that referenced this pull request Jul 19, 2026
Route comment-only members onto the first single-line import and fold
them into combine_star star comments so PyCQA#1852 does not silently drop
body comments on secondary emit paths.

Fixes residual findings from multi-stage review on PyCQA#2588.

Signed-off-by: Alex Chen <l46983284@gmail.com>
@alexchen-sys

Copy link
Copy Markdown
Author

Quick update without the process noise from my last note:

I fixed two edge cases on the current head: force_single_line was dropping body comments, and combine_star left orphan indented comments. Both have regression tests, and the original #1852 black-profile case still passes.

No further product thrash planned from me. Happy to adjust if review wants a different default.

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure the full patch has coverage :)

For the tests I'd prefer the style of the first test so making explicit what the final output looks like instead of using "..." in out. However, if the output is the same as the input please use something like assert isort.code(to_sort, profile="black", line_length=100) == to_sort

@alexchen-sys

Copy link
Copy Markdown
Author

Thanks — I rewrote the #1852 tests to use explicit expected output (including the force_single_line case) and added coverage for the remaining from_body / inject branches.

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix CI to pass? I'll have a look at this afterwards :)

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI still isn't fixed.

alexchen-sys added a commit to alexchen-sys/isort that referenced this pull request Jul 25, 2026
Route comment-only members onto the first single-line import and fold
them into combine_star star comments so PyCQA#1852 does not silently drop
body comments on secondary emit paths.

Fixes residual findings from multi-stage review on PyCQA#2588.

Signed-off-by: Alex Chen <l46983284@gmail.com>
@alexchen-sys
alexchen-sys force-pushed the issue/1852-preserve-comment-only-import-lines branch from 7e0c2ce to 0e3cd7e Compare July 25, 2026 11:52
@alexchen-sys

Copy link
Copy Markdown
Author

Rebased onto main (kept the new alias-comment regression), fixed the lint/format gap, and closed the from_body drops on as-only / force_single emit paths. Local unit suite + mypy/flake8/ruff/hug check are green on this tip.

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing coverage again.

@alexchen-sys

Copy link
Copy Markdown
Author

Thanks — added coverage for the remaining from_body inject branches (ignore_comments, nested-merge fold, and force_single plain-before-as). No product change on this tip.

@alexchen-sys

Copy link
Copy Markdown
Author

Added two regression asserts covering the last uncovered branch in the existing-comment merge (empty segment from a trailing separator, plus the dedup case). Tests only, no product changes — codecov/patch should be at 100% on the diff now.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a regression where comment-only lines inside parenthesized from ... import (...) groups were collapsed onto the opening import line, producing overly long semicolon-joined comments (notably under the black profile) and breaking line-length tooling. The PR introduces a dedicated “from-body comment” channel in parsing and ensures these comments can be re-emitted as their own indented lines inside the import group while preserving legacy behavior for non-parenthesized wrap modes.

Changes:

  • Track comment-only continuation lines inside parenthesized from import groups separately via a new categorized_comments["from_body"] bucket.
  • Re-inject these body comments during output, emitting them as indented lines inside the parentheses (or folding onto the statement when parentheses aren’t used).
  • Add regression tests covering black profile behavior, force_single_line, combine_star, and parser/output branching for issue #1852 (and stability with issue #1396 behavior).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/unit/test_regressions.py Adds regression and unit-level coverage for preserving comment-only lines within parenthesized from-import groups and related edge cases.
isort/parse.py Introduces from_body categorized comments and captures comment-only continuation members during parsing.
isort/output.py Adds _inject_from_body_comments and integrates from_body handling into from-import emission across wrap modes and special cases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread isort/output.py Outdated
Comment on lines 501 to 517
for index, as_import in enumerate(as_import_names):
as_line_comments = list(from_comments) if index == 0 else []
if index == 0 and pending_comments and not emitted_plain:
as_line_comments = pending_comments + as_line_comments
pending_comments = []
output.append(
wrap.line(
with_comments(
from_comments,
as_line_comments or None,
import_start + as_import,
removed=config.ignore_comments,
comment_prefix=config.comment_prefix,
),
parsed.line_separator,
config,
)
for as_import in as_imports[from_import]
)
@DanielNoord

Copy link
Copy Markdown
Member

This will need a rebase unfortunately. It is quite a big PR and is currently not on the top of my priority list for the next release, sorry!

@alexchen-sys
alexchen-sys force-pushed the issue/1852-preserve-comment-only-import-lines branch from e3f9e7f to b383526 Compare August 14, 2026 18:44
Comment-only members of a parenthesised from-import were collapsed onto
the opening import line as a single semicolon-joined comment. With the
black profile that rewrote intentional WIP comments and produced lines
that fail line-length checkers.

Track those body comments separately and re-emit them as indented lines
inside parenthesised wrapping; preserve the historical collapse for
non-parenthesised wrap modes.

Fixes PyCQA#1852

Signed-off-by: Alex Chen <l46983284@gmail.com>
@DanielNoord
DanielNoord force-pushed the issue/1852-preserve-comment-only-import-lines branch from b383526 to 247f001 Compare August 17, 2026 20:52

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, still haven't gotten to this. I did do a rebase on main for you which uncovered an issue, hopefully you can fix it!

captured.append(list(comments or []))
return original_with_comments(comments, *args, **kwargs)

monkeypatch.setattr("isort.output.with_comments", with_comments_probe)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for the mypyc compiled tests. Can we refactr this? If not we'll need to skip it.

The test test_combine_star_folds_body_comments_when_comments_are_enabled_issue_1852
monkeypatches isort.output.with_comments, which cannot work when isort is
compiled with mypyc. Use the house _IS_COMPILED skipif pattern from
tests/unit/test_main.py so the test is skipped in compiled builds.
@alexchen-sys

Copy link
Copy Markdown
Author

Yes — reproduced the failure locally too; I had been testing without mypyc before, so the compiled legs never showed it on my side.

Fixed by skipping the monkeypatch-based test under mypyc, using the same _IS_COMPILED pattern as test_main.py:58. I ran locally on my pc, 13/13 pass; behavior is covered by the sibling e2e test_combine_star_folds_body_comments_issue_1852.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comments in group imports are moved to a single line causing line too long violation (black, flake8)

3 participants