From fe568c08d21405bba46ea14d07a186a7bdd59042 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Wed, 22 Mar 2023 20:00:00 +0200 Subject: [PATCH] isort: fix bad interaction between `force-sort-within-sections` and `force-to-top` (#3645) --- .../isort/force_sort_within_sections.py | 2 + crates/ruff/src/rules/isort/mod.rs | 1 + ..._tests__force_sort_within_sections.py.snap | 6 +- ...ections_force_sort_within_sections.py.snap | 6 +- crates/ruff/src/rules/isort/sorting.rs | 62 ++++++++++--------- 5 files changed, 43 insertions(+), 34 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/isort/force_sort_within_sections.py b/crates/ruff/resources/test/fixtures/isort/force_sort_within_sections.py index 3311cd06d3b2f..a4529de11e208 100644 --- a/crates/ruff/resources/test/fixtures/isort/force_sort_within_sections.py +++ b/crates/ruff/resources/test/fixtures/isort/force_sort_within_sections.py @@ -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 diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index 411f91058e37b..eeebaa4245275 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -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")], diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections.py.snap index 51d77ed4c19a2..b0242e3d44232 100644 --- a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections.py.snap +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections.py.snap @@ -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: ~ diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections_force_sort_within_sections.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections_force_sort_within_sections.py.snap index e73e9874cb6a7..057676a9519f7 100644 --- a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections_force_sort_within_sections.py.snap +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__force_sort_within_sections_force_sort_within_sections.py.snap @@ -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: ~ diff --git a/crates/ruff/src/rules/isort/sorting.rs b/crates/ruff/src/rules/isort/sorting.rs index b59b8639bb367..8e41e54a62810 100644 --- a/crates/ruff/src/rules/isort/sorting.rs +++ b/crates/ruff/src/rules/isort/sorting.rs @@ -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) -> 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, ) -> 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. @@ -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, @@ -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, +) -> 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( @@ -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,