fix: ignore wrapping paren in force_sort_within_sections keys#2592
fix: ignore wrapping paren in force_sort_within_sections keys#2592l46983284-cpu wants to merge 7 commits into
Conversation
Under --fss, multi-line from-imports sort as ``import (Name`` which collates before ``import Alias`` and can separate two plain groups from the same module around an alias (issue PyCQA#2455). Strip the wrapping ``(`` in section_key so those lines sort by the first imported name. Does not merge plain names across alias statements (that path conflicts with PyCQA#2352 trailing plain-after-alias stability). Closes PyCQA#2455 Signed-off-by: Alex Chen <l46983284@gmail.com>
Keep force_sort_within_sections PyCQA#2455 behavior, but move the paren normalization into a small helper so section_key does not pick up an extra cyclomatic-complexity branch for DeepSource PY-R1000. Signed-off-by: Alex Chen <l46983284@gmail.com>
|
DeepSource flagged PY-R1000 on Local: ruff clean on the touched files, |
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
This does not seem to implement the style as agreed upon in the final comments of that issue. Any reason why you deviated from that?
Signed-off-by: Alex Chen <l46983284@gmail.com>
force_sort_within_sections re-sorts emitted statements; rank aliases after plains for the same module so PyCQA#2455 style survives --fss. Signed-off-by: Alex Chen <l46983284@gmail.com>
|
Thanks — I switched this to the agreed style from the final comments on #2455: plain names from the same module are merged into one import, with aliases after. Also fixed --fss so a lexically earlier alias no longer sorts above plains. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2592 +/- ##
==========================================
- Coverage 99.39% 99.36% -0.03%
==========================================
Files 41 41
Lines 3147 3169 +22
Branches 680 685 +5
==========================================
+ Hits 3128 3149 +21
Misses 11 11
- Partials 8 9 +1 🚀 New features to boost your workflow:
|
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
Instead of keeping track of these cases (in this quite complex code), wouldn't it be easier to merge import statements at the end? That seems like it would be simpler to understand and maintain
Reformat isort's own sources for the plain-before-alias grouping so the hug-profile lint check passes. Skip django/habitat-lab/pylint integration baselines that intentionally change under the PyCQA#2455 preferred style until those projects re-sort. Keep alias_rank only on non-lexicographical keys and harden multi-line paren stripping for section_key. Signed-off-by: Alex Chen <l46983284@gmail.com>
Drop mid-path defer_as bookkeeping. After statements are emitted, merge comment-free plain from-import statements for the same module and put that plain group before free aliases when needed. Fixes review feedback on PyCQA#2592 to keep mid-path emit simple. Signed-off-by: Alex Chen <l46983284@gmail.com>
After dropping defer_as bookkeeping, the combine_as condition still referenced a stale from_import name from an earlier loop. Use the current head of from_imports like the previous preferred tip. Signed-off-by: Alex Chen <l46983284@gmail.com>
DanielNoord
left a comment
DanielNoord
left a comment
There was a problem hiding this comment.
This seems like it changes our style signficantly. I think we should first discuss other alternatives in the issue. Unfortunately the style as discussed there woulkd be a breaking change for the project.
|
No pressure at all — feel free to close this PR or reuse any part of the code if it's useful. It was a fun exercise to work through. If you have thoughts on what direction would make sense for the issue, I'd be happy to collaborate on a follow-up. |
|
I'll close for now, thanks for having a stab at this! |
2 participants
Summary
Under
--force-sort_within_sections, multi-linefromimports were sorted asimport (/ newline-indented names, which collates beforeimport Aliasand can pull a later plain group ahead of an alias from the same module (issue #2455).This normalizes the wrapping parenthesis and following whitespace in
section_keyso those lines sort by the first imported name, matching single-line imports.Non-goals
Does not merge separate plain
from module import (...)statements across an intermediate alias. That larger contract change conflicts with #2352 trailing plain-after-alias stability. Issue #2455 listed thissection_keynormalization as a valid approach.Test plan
tests/unit/test_regressions.py::test_from_import_alias_does_not_split_plain_names_issue_2455(default GRID + black multi-line under fss)isortis not stable on a file which imports multiple members from the same module, one aliased and another imported directly, with--profile black#2352 trailing-comma mixed-as tests still passCloses #2455