From 941e0d29ffda48542eb1ae45b5d667debfb1c550 Mon Sep 17 00:00:00 2001 From: Florian Best Date: Mon, 20 Feb 2023 18:50:42 +0100 Subject: [PATCH] fix(isort): comply with isort when sorting sub-package modules e.g. packages named "foo.bar" additionally make sure `__future__` imports are always sorted at the very top. Issue #3059 --- .../test/fixtures/isort/pyproject.toml | 2 - ...ubpackage_first_and_third_party_imports.py | 8 ++++ .../flake8_tidy_imports/relative_imports.rs | 2 +- .../rules/typing_only_runtime_import.rs | 6 +-- crates/ruff/src/rules/isort/categorize.rs | 25 +++++++---- crates/ruff/src/rules/isort/mod.rs | 41 +++++++++++++++++++ ...kage_first_and_third_party_imports.py.snap | 22 ++++++++++ ...kage_first_and_third_party_imports.py.snap | 22 ++++++++++ 8 files changed, 113 insertions(+), 15 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/isort/separate_subpackage_first_and_third_party_imports.py create mode 100644 crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__1_separate_subpackage_first_and_third_party_imports.py.snap create mode 100644 crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__2_separate_subpackage_first_and_third_party_imports.py.snap diff --git a/crates/ruff/resources/test/fixtures/isort/pyproject.toml b/crates/ruff/resources/test/fixtures/isort/pyproject.toml index 63e7153b20c7d..4a2fa0f7220fb 100644 --- a/crates/ruff/resources/test/fixtures/isort/pyproject.toml +++ b/crates/ruff/resources/test/fixtures/isort/pyproject.toml @@ -4,5 +4,3 @@ line-length = 88 [tool.ruff.isort] lines-after-imports = 3 lines-between-types = 2 -known-local-folder = ["ruff"] -force-to-top = ["lib1", "lib3", "lib5", "lib3.lib4", "z"] diff --git a/crates/ruff/resources/test/fixtures/isort/separate_subpackage_first_and_third_party_imports.py b/crates/ruff/resources/test/fixtures/isort/separate_subpackage_first_and_third_party_imports.py new file mode 100644 index 0000000000000..8ea1742464f2b --- /dev/null +++ b/crates/ruff/resources/test/fixtures/isort/separate_subpackage_first_and_third_party_imports.py @@ -0,0 +1,8 @@ +import sys +import baz +from foo import bar, baz +from foo.bar import blah, blub +from foo.bar.baz import something +import foo +import foo.bar +import foo.bar.baz diff --git a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs index c95f66ef3ed65..216194d44965b 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs @@ -92,7 +92,7 @@ fn fix_banned_relative_import( module_path: Option<&Vec>, stylist: &Stylist, ) -> Option { - // Only fix is the module path is known. + // Only fix if the module path is known. if let Some(mut parts) = module_path.cloned() { // Remove relative level from module path. for _ in 0..*level? { diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 32703f40c7e79..5f3b3e70b6168 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -150,15 +150,13 @@ pub fn typing_only_runtime_import( && binding.runtime_usage.is_none() && binding.synthetic_usage.is_none() { - // Extract the module base and level from the full name. - // Ex) `foo.bar.baz` -> `foo`, `0` + // Extract the module level from the full name. // Ex) `.foo.bar.baz` -> `foo`, `1` - let module_base = full_name.split('.').next().unwrap(); let level = full_name.chars().take_while(|c| *c == '.').count(); // Categorize the import. match categorize( - module_base, + full_name, Some(&level), &settings.src, package, diff --git a/crates/ruff/src/rules/isort/categorize.rs b/crates/ruff/src/rules/isort/categorize.rs index b2fd90cf1b763..514a436b7ac38 100644 --- a/crates/ruff/src/rules/isort/categorize.rs +++ b/crates/ruff/src/rules/isort/categorize.rs @@ -38,7 +38,7 @@ enum Reason<'a> { #[allow(clippy::too_many_arguments)] pub fn categorize( - module_base: &str, + module_name: &str, level: Option<&usize>, src: &[PathBuf], package: Option<&Path>, @@ -48,9 +48,20 @@ pub fn categorize( extra_standard_library: &BTreeSet, target_version: PythonVersion, ) -> ImportType { + let module_base = module_name.split('.').next().unwrap(); let (import_type, reason) = { if level.map_or(false, |level| *level > 0) { (ImportType::LocalFolder, Reason::NonZeroLevel) + } else if module_base == "__future__" { + (ImportType::Future, Reason::Future) + } else if known_first_party.contains(module_name) { + (ImportType::FirstParty, Reason::KnownFirstParty) + } else if known_third_party.contains(module_name) { + (ImportType::ThirdParty, Reason::KnownThirdParty) + } else if known_local_folder.contains(module_name) { + (ImportType::LocalFolder, Reason::KnownLocalFolder) + } else if extra_standard_library.contains(module_name) { + (ImportType::StandardLibrary, Reason::ExtraStandardLibrary) } else if known_first_party.contains(module_base) { (ImportType::FirstParty, Reason::KnownFirstParty) } else if known_third_party.contains(module_base) { @@ -59,8 +70,6 @@ pub fn categorize( (ImportType::LocalFolder, Reason::KnownLocalFolder) } else if extra_standard_library.contains(module_base) { (ImportType::StandardLibrary, Reason::ExtraStandardLibrary) - } else if module_base == "__future__" { - (ImportType::Future, Reason::Future) } else if KNOWN_STANDARD_LIBRARY .get(&target_version.as_tuple()) .unwrap() @@ -77,7 +86,7 @@ pub fn categorize( }; debug!( "Categorized '{}' as {:?} ({:?})", - module_base, import_type, reason + module_name, import_type, reason ); import_type } @@ -117,7 +126,7 @@ pub fn categorize_imports<'a>( // Categorize `StmtKind::Import`. for (alias, comments) in block.import { let import_type = categorize( - &alias.module_base(), + &alias.module_name(), None, src, package, @@ -136,7 +145,7 @@ pub fn categorize_imports<'a>( // Categorize `StmtKind::ImportFrom` (without re-export). for (import_from, aliases) in block.import_from { let classification = categorize( - &import_from.module_base(), + &import_from.module_name(), import_from.level, src, package, @@ -155,7 +164,7 @@ pub fn categorize_imports<'a>( // Categorize `StmtKind::ImportFrom` (with re-export). for ((import_from, alias), comments) in block.import_from_as { let classification = categorize( - &import_from.module_base(), + &import_from.module_name(), import_from.level, src, package, @@ -174,7 +183,7 @@ pub fn categorize_imports<'a>( // Categorize `StmtKind::ImportFrom` (with star). for (import_from, comments) in block.import_from_star { let classification = categorize( - &import_from.module_base(), + &import_from.module_name(), import_from.level, src, package, diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index 1089f6a6ae006..0775d280de63e 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -400,6 +400,47 @@ mod tests { Ok(()) } + #[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))] + fn separate_modules(path: &Path) -> Result<()> { + let snapshot = format!("1_{}", path.to_string_lossy()); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &Settings { + isort: super::settings::Settings { + known_first_party: BTreeSet::from(["foo.bar".to_string(), "baz".to_string()]), + known_third_party: BTreeSet::from([ + "foo".to_string(), + "__future__".to_string(), + ]), + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..Settings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_yaml_snapshot!(snapshot, diagnostics); + Ok(()) + } + + #[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))] + fn separate_modules_first_party(path: &Path) -> Result<()> { + let snapshot = format!("2_{}", path.to_string_lossy()); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &Settings { + isort: super::settings::Settings { + known_first_party: BTreeSet::from(["foo".to_string()]), + known_third_party: BTreeSet::from(["foo.bar".to_string()]), + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..Settings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_yaml_snapshot!(snapshot, diagnostics); + Ok(()) + } + // Test currently disabled as line endings are automatically converted to // platform-appropriate ones in CI/CD #[test_case(Path::new(" // line_ending_crlf.py"))] #[test_case(Path::new("line_ending_lf.py"))] diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__1_separate_subpackage_first_and_third_party_imports.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__1_separate_subpackage_first_and_third_party_imports.py.snap new file mode 100644 index 0000000000000..e289fec99101d --- /dev/null +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__1_separate_subpackage_first_and_third_party_imports.py.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +expression: diagnostics +--- +- kind: + UnsortedImports: ~ + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + fix: + content: "import sys\n\nimport foo\nfrom foo import bar, baz\n\nimport baz\nimport foo.bar\nimport foo.bar.baz\nfrom foo.bar import blah, blub\nfrom foo.bar.baz import something\n" + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + parent: ~ + diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__2_separate_subpackage_first_and_third_party_imports.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__2_separate_subpackage_first_and_third_party_imports.py.snap new file mode 100644 index 0000000000000..14131bd39d3d1 --- /dev/null +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__2_separate_subpackage_first_and_third_party_imports.py.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +expression: diagnostics +--- +- kind: + UnsortedImports: ~ + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + fix: + content: "import sys\n\nimport baz\nimport foo.bar\nimport foo.bar.baz\nfrom foo.bar import blah, blub\nfrom foo.bar.baz import something\n\nimport foo\nfrom foo import bar, baz\n" + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + parent: ~ +