-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression in Ruff's ordering of import aliases with force-sort-within-sections
#8661
Comments
Thanks! @bluthej, are you able to take a look at this? If not, I understand and can investigate myself, but thought I would ask first since it's likely related to your recent refactor. |
Regardless, @cmsetzer, will make sure this is fixed for the next release. Thanks for the clear write-up. |
@charliermarsh sure I'll take a look at it! |
@bluthej - Thank you! |
Ok so what happens is the from imports don't have an "asname", so they have a Looks like inverting the last two fields of |
Fixes #8661 ## Summary Imports like `from x import y` don't have an "asname" for the module, so they were placed before imports like `import x as w` since `None` < `Some(s)` for any string s. The fix is to first sort by `first_alias`, since it's `None` for `import x as w`, and then by `asname`. ## Test Plan I included the example from the issue to avoid future regressions.
While fixing #8661 I noticed that the code structure for sorting imports could be simplified. ## Summary - Move the logic for `force_sort_within_sections` from `isort/mod.rs` to `isort/ordering.rs` => now there is just one line in `isort/mod.rs`: `let imports = order_imports(import_block, settings);` which yields the sorted imports - Change the function signature of `order_imports` to directly return a `Vec<EitherImport<'a>>` => no need for `OrderedImportBlock` I think this is a bit of an improvement because the code is simpler and there should be a bit of a speedup when setting `force-sort-within-sections` to true. Indeed, when it's set to true we're now directly ordering all the imports, whereas before we would first order the straight imports, then the from imports, combine them and finally sort the combination a second time (this is probably not noticeable in practice though). ## Test Plan No tests added, this is a simple refactor.
It looks like #7963 introduced a regression in Ruff's ordering of certain import aliases when
force-sort-within-sections
is enabled. In version 0.1.3 and prior, Ruff and isort produced identical output under this setting.Example
Given
pyproject.toml
:And
example.py
:isort produces the following:
...while Ruff now places
import datetime as dt
out of order:The text was updated successfully, but these errors were encountered: