Skip to content
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

isort: fix bad interaction between force-sort-within-sections and force-to-top #3645

Merged
merged 2 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
from c import * # import_from_star
import a # import
import c.d
from z import z1
import b as b1 # import_as
import z

from ..parent import *
from .my import fn
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ mod tests {
&Settings {
isort: super::settings::Settings {
force_sort_within_sections: true,
force_to_top: BTreeSet::from(["z".to_string()]),
..super::settings::Settings::default()
},
src: vec![test_resource_path("fixtures/isort")],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ expression: diagnostics
row: 1
column: 0
end_location:
row: 12
row: 14
column: 0
fix:
content: "import a # import\nimport b as b1 # import_as\nimport c.d\nfrom a import a1 # import_from\nfrom c import * # import_from_star\n\nfrom ...grandparent import fn3\nfrom ..parent import *\nfrom . import my\nfrom .my import fn\nfrom .my.nested import fn2\n"
content: "import a # import\nimport b as b1 # import_as\nimport c.d\nimport z\nfrom a import a1 # import_from\nfrom c import * # import_from_star\nfrom z import z1\n\nfrom ...grandparent import fn3\nfrom ..parent import *\nfrom . import my\nfrom .my import fn\nfrom .my.nested import fn2\n"
location:
row: 1
column: 0
end_location:
row: 12
row: 14
column: 0
parent: ~

Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ expression: diagnostics
row: 1
column: 0
end_location:
row: 12
row: 14
column: 0
fix:
content: "import a # import\nfrom a import a1 # import_from\nimport b as b1 # import_as\nfrom c import * # import_from_star\nimport c.d\n\nfrom ...grandparent import fn3\nfrom ..parent import *\nfrom . import my\nfrom .my import fn\nfrom .my.nested import fn2\n"
content: "import z\nfrom z import z1\nimport a # import\nfrom a import a1 # import_from\nimport b as b1 # import_as\nfrom c import * # import_from_star\nimport c.d\n\nfrom ...grandparent import fn3\nfrom ..parent import *\nfrom . import my\nfrom .my import fn\nfrom .my.nested import fn2\n"
location:
row: 1
column: 0
end_location:
row: 12
row: 14
column: 0
parent: ~

62 changes: 34 additions & 28 deletions crates/ruff/src/rules/isort/sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,29 +44,28 @@ fn prefix(
}
}

/// Compare two module names' by their `force-to-top`ness.
fn cmp_force_to_top(name1: &str, name2: &str, force_to_top: &BTreeSet<String>) -> Ordering {
let force_to_top1 = force_to_top.contains(name1);
let force_to_top2 = force_to_top.contains(name2);
force_to_top1.cmp(&force_to_top2).reverse()
}

/// Compare two top-level modules.
pub fn cmp_modules(
alias1: &AliasData,
alias2: &AliasData,
force_to_top: &BTreeSet<String>,
) -> Ordering {
(match (
force_to_top.contains(alias1.name),
force_to_top.contains(alias2.name),
) {
(true, true) => Ordering::Equal,
(false, false) => Ordering::Equal,
(true, false) => Ordering::Less,
(false, true) => Ordering::Greater,
})
.then_with(|| natord::compare_ignore_case(alias1.name, alias2.name))
.then_with(|| natord::compare(alias1.name, alias2.name))
.then_with(|| match (alias1.asname, alias2.asname) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(asname1), Some(asname2)) => natord::compare(asname1, asname2),
})
cmp_force_to_top(alias1.name, alias2.name, force_to_top)
.then_with(|| natord::compare_ignore_case(alias1.name, alias2.name))
.then_with(|| natord::compare(alias1.name, alias2.name))
.then_with(|| match (alias1.asname, alias2.asname) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(asname1), Some(asname2)) => natord::compare(asname1, asname2),
})
}

/// Compare two member imports within `StmtKind::ImportFrom` blocks.
Expand Down Expand Up @@ -124,15 +123,11 @@ pub fn cmp_import_from(
relative_imports_order,
)
.then_with(|| {
match (
force_to_top.contains(&import_from1.module_name()),
force_to_top.contains(&import_from2.module_name()),
) {
(true, true) => Ordering::Equal,
(false, false) => Ordering::Equal,
(true, false) => Ordering::Less,
(false, true) => Ordering::Greater,
}
cmp_force_to_top(
&import_from1.module_name(),
&import_from2.module_name(),
force_to_top,
)
})
.then_with(|| match (&import_from1.module, import_from2.module) {
(None, None) => Ordering::Equal,
Expand All @@ -143,6 +138,17 @@ pub fn cmp_import_from(
})
}

/// Compare an import to an import-from.
fn cmp_import_import_from(
import: &AliasData,
import_from: &ImportFromData,
force_to_top: &BTreeSet<String>,
) -> Ordering {
cmp_force_to_top(import.name, &import_from.module_name(), force_to_top).then_with(|| {
natord::compare_ignore_case(import.name, import_from.module.unwrap_or_default())
})
}

/// Compare two [`EitherImport`] enums which may be [`Import`] or [`ImportFrom`]
/// structs.
pub fn cmp_either_import(
Expand All @@ -154,10 +160,10 @@ pub fn cmp_either_import(
match (a, b) {
(Import((alias1, _)), Import((alias2, _))) => cmp_modules(alias1, alias2, force_to_top),
(ImportFrom((import_from, ..)), Import((alias, _))) => {
natord::compare_ignore_case(import_from.module.unwrap_or_default(), alias.name)
cmp_import_import_from(alias, import_from, force_to_top).reverse()
}
(Import((alias, _)), ImportFrom((import_from, ..))) => {
natord::compare_ignore_case(alias.name, import_from.module.unwrap_or_default())
cmp_import_import_from(alias, import_from, force_to_top)
}
(ImportFrom((import_from1, ..)), ImportFrom((import_from2, ..))) => cmp_import_from(
import_from1,
Expand Down