From ececab131eb2dea75c254ff644db57f36f456410 Mon Sep 17 00:00:00 2001 From: Anze Staric Date: Tue, 28 Mar 2023 11:47:23 +0200 Subject: [PATCH 1/3] fix(isort): support submodules in known_*_party Allow submodules to be specified in know_first_party and known_third_party config options. When two entries in config options share the same prefix, the longest match wins. Fixes #3059 --- ...ubpackage_first_and_third_party_imports.py | 8 ++ .../rules/typing_only_runtime_import.rs | 6 +- crates/ruff/src/rules/isort/categorize.rs | 94 ++++++++++++++----- crates/ruff/src/rules/isort/mod.rs | 53 +++++++++-- .../src/rules/isort/rules/organize_imports.rs | 3 +- crates/ruff/src/rules/isort/settings.rs | 17 ++-- ...kage_first_and_third_party_imports.py.snap | 26 +++++ ...kage_first_and_third_party_imports.py.snap | 26 +++++ 8 files changed, 189 insertions(+), 44 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/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_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index b20dee682d6a8..f3cb2a1bcbe5c 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 @@ -178,17 +178,15 @@ pub fn typing_only_runtime_import( // Extract the module base and level from the full name. // Ex) `foo.bar.baz` -> `foo`, `0` // 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, - &settings.isort.known_first_party, - &settings.isort.known_third_party, + &settings.isort.known_modules, &settings.isort.known_local_folder, &settings.isort.extra_standard_library, settings.target_version, diff --git a/crates/ruff/src/rules/isort/categorize.rs b/crates/ruff/src/rules/isort/categorize.rs index 7cb33df78475f..4e8d2b64bfcdf 100644 --- a/crates/ruff/src/rules/isort/categorize.rs +++ b/crates/ruff/src/rules/isort/categorize.rs @@ -52,23 +52,21 @@ 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>, - known_first_party: &BTreeSet, - known_third_party: &BTreeSet, + known_modules: &KnownModules, known_local_folder: &BTreeSet, 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 known_first_party.contains(module_base) { - (ImportType::FirstParty, Reason::KnownFirstParty) - } else if known_third_party.contains(module_base) { - (ImportType::ThirdParty, Reason::KnownThirdParty) + } else if let Some(type_and_reason) = known_modules.get_category(module_name) { + type_and_reason } else if known_local_folder.contains(module_base) { (ImportType::LocalFolder, Reason::KnownLocalFolder) } else if extra_standard_library.contains(module_base) { @@ -91,7 +89,7 @@ pub fn categorize( }; debug!( "Categorized '{}' as {:?} ({:?})", - module_base, import_type, reason + module_name, import_type, reason ); import_type } @@ -121,8 +119,7 @@ pub fn categorize_imports<'a>( block: ImportBlock<'a>, src: &[PathBuf], package: Option<&Path>, - known_first_party: &BTreeSet, - known_third_party: &BTreeSet, + known_modules: &KnownModules, known_local_folder: &BTreeSet, extra_standard_library: &BTreeSet, target_version: PythonVersion, @@ -131,12 +128,11 @@ 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, - known_first_party, - known_third_party, + known_modules, known_local_folder, extra_standard_library, target_version, @@ -150,12 +146,11 @@ 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, - known_first_party, - known_third_party, + known_modules, known_local_folder, extra_standard_library, target_version, @@ -169,12 +164,11 @@ pub fn categorize_imports<'a>( // Categorize `StmtKind::ImportFrom` (with re-export). for ((import_from, alias), aliases) in block.import_from_as { let classification = categorize( - &import_from.module_base(), + &import_from.module_name(), import_from.level, src, package, - known_first_party, - known_third_party, + known_modules, known_local_folder, extra_standard_library, target_version, @@ -188,12 +182,11 @@ 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, - known_first_party, - known_third_party, + known_modules, known_local_folder, extra_standard_library, target_version, @@ -206,3 +199,60 @@ pub fn categorize_imports<'a>( } block_by_type } + +#[derive(Debug, Default, CacheKey)] +pub struct KnownModules { + pub first_party: BTreeSet, + pub third_party: BTreeSet, + has_submodules: bool, +} + +impl KnownModules { + pub fn new>(first_party: T, third_party: T) -> Self { + let first_party = BTreeSet::from_iter(first_party); + let third_party = BTreeSet::from_iter(third_party); + let fp_submodules = first_party.iter().filter(|m| m.contains('.')).count() != 0; + let tp_submodules = third_party.iter().filter(|m| m.contains('.')).count() != 0; + Self { + first_party, + third_party, + has_submodules: fp_submodules || tp_submodules, + } + } + + fn get_category(&self, module_name: &str) -> Option<(ImportType, Reason)> { + // Shortcut for everyone that does not use submodules in KnownModules + if !self.has_submodules { + let module_base = module_name.split('.').next().unwrap(); + if self.first_party.contains(module_base) { + return Some((ImportType::FirstParty, Reason::KnownFirstParty)); + } + if self.third_party.contains(module_base) { + return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); + } + } + + // Check all module prefixes from the longest to the shortest. The first one + // matching a value in either first_party or third_party modules defines + // the category. + let parts: Vec = module_name + .chars() + .enumerate() + .filter(|(_, c)| *c == '.') + .map(|(i, _)| i) + .chain([module_name.len()]) + .collect(); + + for i in parts.iter().rev() { + let submodule = &module_name[0..*i]; + if self.first_party.contains(submodule) { + return Some((ImportType::FirstParty, Reason::KnownFirstParty)); + } + if self.third_party.contains(submodule) { + return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); + } + } + + None + } +} diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index eeebaa4245275..79a0986c05081 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -6,6 +6,7 @@ use std::path::{Path, PathBuf}; use itertools::Either::{Left, Right}; use strum::IntoEnumIterator; +use crate::rules::isort::categorize::KnownModules; use annotate::annotate_imports; use categorize::categorize_imports; pub use categorize::{categorize, ImportType}; @@ -123,8 +124,7 @@ pub fn format_imports( force_sort_within_sections: bool, force_wrap_aliases: bool, force_to_top: &BTreeSet, - known_first_party: &BTreeSet, - known_third_party: &BTreeSet, + known_modules: &KnownModules, known_local_folder: &BTreeSet, order_by_type: bool, relative_imports_order: RelativeImportsOrder, @@ -159,8 +159,7 @@ pub fn format_imports( force_sort_within_sections, force_wrap_aliases, force_to_top, - known_first_party, - known_third_party, + known_modules, known_local_folder, order_by_type, relative_imports_order, @@ -219,8 +218,7 @@ fn format_import_block( force_sort_within_sections: bool, force_wrap_aliases: bool, force_to_top: &BTreeSet, - known_first_party: &BTreeSet, - known_third_party: &BTreeSet, + known_modules: &KnownModules, known_local_folder: &BTreeSet, order_by_type: bool, relative_imports_order: RelativeImportsOrder, @@ -238,8 +236,7 @@ fn format_import_block( block, src, package, - known_first_party, - known_third_party, + known_modules, known_local_folder, extra_standard_library, target_version, @@ -352,6 +349,7 @@ mod tests { use test_case::test_case; use crate::registry::Rule; + use crate::rules::isort::categorize::KnownModules; use crate::settings::Settings; use crate::test::{test_path, test_resource_path}; @@ -416,6 +414,45 @@ 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_modules: KnownModules::new( + ["foo.bar".to_string(), "baz".to_string()], + ["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_modules: KnownModules::new(["foo".to_string()], ["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/rules/organize_imports.rs b/crates/ruff/src/rules/isort/rules/organize_imports.rs index f71c50266b21b..7e6cf5d27c12b 100644 --- a/crates/ruff/src/rules/isort/rules/organize_imports.rs +++ b/crates/ruff/src/rules/isort/rules/organize_imports.rs @@ -127,8 +127,7 @@ pub fn organize_imports( settings.isort.force_sort_within_sections, settings.isort.force_wrap_aliases, &settings.isort.force_to_top, - &settings.isort.known_first_party, - &settings.isort.known_third_party, + &settings.isort.known_modules, &settings.isort.known_local_folder, settings.isort.order_by_type, settings.isort.relative_imports_order, diff --git a/crates/ruff/src/rules/isort/settings.rs b/crates/ruff/src/rules/isort/settings.rs index 4fe3f001444dd..8b78291ac0b48 100644 --- a/crates/ruff/src/rules/isort/settings.rs +++ b/crates/ruff/src/rules/isort/settings.rs @@ -5,6 +5,7 @@ use std::collections::BTreeSet; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use crate::rules::isort::categorize::KnownModules; use ruff_macros::{CacheKey, ConfigurationOptions}; use super::categorize::ImportType; @@ -276,8 +277,7 @@ pub struct Settings { pub force_sort_within_sections: bool, pub force_wrap_aliases: bool, pub force_to_top: BTreeSet, - pub known_first_party: BTreeSet, - pub known_third_party: BTreeSet, + pub known_modules: KnownModules, pub known_local_folder: BTreeSet, pub order_by_type: bool, pub relative_imports_order: RelativeImportsOrder, @@ -302,8 +302,7 @@ impl Default for Settings { force_sort_within_sections: false, force_wrap_aliases: false, force_to_top: BTreeSet::new(), - known_first_party: BTreeSet::new(), - known_third_party: BTreeSet::new(), + known_modules: KnownModules::default(), known_local_folder: BTreeSet::new(), order_by_type: true, relative_imports_order: RelativeImportsOrder::default(), @@ -332,8 +331,10 @@ impl From for Settings { force_sort_within_sections: options.force_sort_within_sections.unwrap_or(false), force_wrap_aliases: options.force_wrap_aliases.unwrap_or(false), force_to_top: BTreeSet::from_iter(options.force_to_top.unwrap_or_default()), - known_first_party: BTreeSet::from_iter(options.known_first_party.unwrap_or_default()), - known_third_party: BTreeSet::from_iter(options.known_third_party.unwrap_or_default()), + known_modules: KnownModules::new( + options.known_first_party.unwrap_or_default(), + options.known_third_party.unwrap_or_default(), + ), known_local_folder: BTreeSet::from_iter(options.known_local_folder.unwrap_or_default()), order_by_type: options.order_by_type.unwrap_or(true), relative_imports_order: options.relative_imports_order.unwrap_or_default(), @@ -362,8 +363,8 @@ impl From for Options { force_sort_within_sections: Some(settings.force_sort_within_sections), force_wrap_aliases: Some(settings.force_wrap_aliases), force_to_top: Some(settings.force_to_top.into_iter().collect()), - known_first_party: Some(settings.known_first_party.into_iter().collect()), - known_third_party: Some(settings.known_third_party.into_iter().collect()), + known_first_party: Some(settings.known_modules.first_party.into_iter().collect()), + known_third_party: Some(settings.known_modules.third_party.into_iter().collect()), known_local_folder: Some(settings.known_local_folder.into_iter().collect()), order_by_type: Some(settings.order_by_type), relative_imports_order: Some(settings.relative_imports_order), 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..05fae1b75ca58 --- /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,26 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +expression: diagnostics +--- +- kind: + name: UnsortedImports + body: Import block is un-sorted or un-formatted + suggestion: Organize imports + fixable: true + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + fix: + edits: + - 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..b46518b172cd9 --- /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,26 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +expression: diagnostics +--- +- kind: + name: UnsortedImports + body: Import block is un-sorted or un-formatted + suggestion: Organize imports + fixable: true + location: + row: 1 + column: 0 + end_location: + row: 9 + column: 0 + fix: + edits: + - 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: ~ + From 4f509323bec18ee74f0cfae9efc262254d971a74 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 28 Mar 2023 23:45:45 -0400 Subject: [PATCH 2/3] Extend to known-local-folder and extra-standard-library --- .../rules/typing_only_runtime_import.rs | 2 - crates/ruff/src/rules/isort/categorize.rs | 88 +++++++++++-------- crates/ruff/src/rules/isort/mod.rs | 36 ++++---- .../src/rules/isort/rules/organize_imports.rs | 2 - crates/ruff/src/rules/isort/settings.rs | 20 ++--- 5 files changed, 78 insertions(+), 70 deletions(-) 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 f3cb2a1bcbe5c..f46875d26650f 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 @@ -187,8 +187,6 @@ pub fn typing_only_runtime_import( &settings.src, package, &settings.isort.known_modules, - &settings.isort.known_local_folder, - &settings.isort.extra_standard_library, settings.target_version, ) { ImportType::LocalFolder | ImportType::FirstParty => Some(Diagnostic::new( diff --git a/crates/ruff/src/rules/isort/categorize.rs b/crates/ruff/src/rules/isort/categorize.rs index 4e8d2b64bfcdf..ea34bc975a61d 100644 --- a/crates/ruff/src/rules/isort/categorize.rs +++ b/crates/ruff/src/rules/isort/categorize.rs @@ -1,6 +1,6 @@ use std::collections::{BTreeMap, BTreeSet}; -use std::fs; use std::path::{Path, PathBuf}; +use std::{fs, iter}; use log::debug; use schemars::JsonSchema; @@ -57,22 +57,16 @@ pub fn categorize( src: &[PathBuf], package: Option<&Path>, known_modules: &KnownModules, - known_local_folder: &BTreeSet, - 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 let Some(type_and_reason) = known_modules.get_category(module_name) { - type_and_reason - } else if known_local_folder.contains(module_base) { - (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 let Some((import_type, reason)) = known_modules.categorize(module_name) { + (import_type, reason) } else if KNOWN_STANDARD_LIBRARY .get(&target_version.as_tuple()) .unwrap() @@ -120,8 +114,6 @@ pub fn categorize_imports<'a>( src: &[PathBuf], package: Option<&Path>, known_modules: &KnownModules, - known_local_folder: &BTreeSet, - extra_standard_library: &BTreeSet, target_version: PythonVersion, ) -> BTreeMap> { let mut block_by_type: BTreeMap = BTreeMap::default(); @@ -133,8 +125,6 @@ pub fn categorize_imports<'a>( src, package, known_modules, - known_local_folder, - extra_standard_library, target_version, ); block_by_type @@ -151,8 +141,6 @@ pub fn categorize_imports<'a>( src, package, known_modules, - known_local_folder, - extra_standard_library, target_version, ); block_by_type @@ -169,8 +157,6 @@ pub fn categorize_imports<'a>( src, package, known_modules, - known_local_folder, - extra_standard_library, target_version, ); block_by_type @@ -187,8 +173,6 @@ pub fn categorize_imports<'a>( src, package, known_modules, - known_local_folder, - extra_standard_library, target_version, ); block_by_type @@ -202,26 +186,48 @@ pub fn categorize_imports<'a>( #[derive(Debug, Default, CacheKey)] pub struct KnownModules { + /// A set of user-provided first-party modules. pub first_party: BTreeSet, + /// A set of user-provided third-party modules. pub third_party: BTreeSet, + /// A set of user-provided local folder modules. + pub local_folder: BTreeSet, + /// A set of user-provided standard library modules. + pub standard_library: BTreeSet, + /// Whether any of the known modules are submodules (e.g., `foo.bar`, as opposed to `foo`). has_submodules: bool, } impl KnownModules { - pub fn new>(first_party: T, third_party: T) -> Self { + pub fn new( + first_party: Vec, + third_party: Vec, + local_folder: Vec, + standard_library: Vec, + ) -> Self { let first_party = BTreeSet::from_iter(first_party); let third_party = BTreeSet::from_iter(third_party); - let fp_submodules = first_party.iter().filter(|m| m.contains('.')).count() != 0; - let tp_submodules = third_party.iter().filter(|m| m.contains('.')).count() != 0; + let local_folder = BTreeSet::from_iter(local_folder); + let standard_library = BTreeSet::from_iter(standard_library); + let has_submodules = first_party + .iter() + .chain(third_party.iter()) + .chain(local_folder.iter()) + .chain(standard_library.iter()) + .any(|m| m.contains('.')); Self { first_party, third_party, - has_submodules: fp_submodules || tp_submodules, + local_folder, + standard_library, + has_submodules, } } - fn get_category(&self, module_name: &str) -> Option<(ImportType, Reason)> { - // Shortcut for everyone that does not use submodules in KnownModules + /// Return the [`ImportType`] for a given module, if it's been categorized as a known module + /// by the user. + fn categorize(&self, module_name: &str) -> Option<(ImportType, Reason)> { + // Happy path: no submodules, so we can check the module base and be done. if !self.has_submodules { let module_base = module_name.split('.').next().unwrap(); if self.first_party.contains(module_base) { @@ -230,27 +236,35 @@ impl KnownModules { if self.third_party.contains(module_base) { return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); } + if self.local_folder.contains(module_base) { + return Some((ImportType::LocalFolder, Reason::KnownLocalFolder)); + } + if self.standard_library.contains(module_base) { + return Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)); + } } - // Check all module prefixes from the longest to the shortest. The first one - // matching a value in either first_party or third_party modules defines - // the category. - let parts: Vec = module_name - .chars() - .enumerate() - .filter(|(_, c)| *c == '.') + // Check all module prefixes from the longest to the shortest (e.g., given `foo.bar.baz`, + // check `foo.bar.baz`, then `foo.bar`, then `foo`, taking the first, most precise match). + for i in module_name + .match_indices('.') .map(|(i, _)| i) - .chain([module_name.len()]) - .collect(); - - for i in parts.iter().rev() { - let submodule = &module_name[0..*i]; + .chain(iter::once(module_name.len())) + .rev() + { + let submodule = &module_name[0..i]; if self.first_party.contains(submodule) { return Some((ImportType::FirstParty, Reason::KnownFirstParty)); } if self.third_party.contains(submodule) { return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); } + if self.local_folder.contains(submodule) { + return Some((ImportType::LocalFolder, Reason::KnownLocalFolder)); + } + if self.standard_library.contains(submodule) { + return Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)); + } } None diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index 79a0986c05081..71fa5926b39db 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -119,13 +119,11 @@ pub fn format_imports( src: &[PathBuf], package: Option<&Path>, combine_as_imports: bool, - extra_standard_library: &BTreeSet, force_single_line: bool, force_sort_within_sections: bool, force_wrap_aliases: bool, force_to_top: &BTreeSet, known_modules: &KnownModules, - known_local_folder: &BTreeSet, order_by_type: bool, relative_imports_order: RelativeImportsOrder, single_line_exclusions: &BTreeSet, @@ -154,13 +152,11 @@ pub fn format_imports( stylist, src, package, - extra_standard_library, force_single_line, force_sort_within_sections, force_wrap_aliases, force_to_top, known_modules, - known_local_folder, order_by_type, relative_imports_order, single_line_exclusions, @@ -213,13 +209,11 @@ fn format_import_block( stylist: &Stylist, src: &[PathBuf], package: Option<&Path>, - extra_standard_library: &BTreeSet, force_single_line: bool, force_sort_within_sections: bool, force_wrap_aliases: bool, force_to_top: &BTreeSet, known_modules: &KnownModules, - known_local_folder: &BTreeSet, order_by_type: bool, relative_imports_order: RelativeImportsOrder, single_line_exclusions: &BTreeSet, @@ -232,15 +226,7 @@ fn format_import_block( target_version: PythonVersion, ) -> String { // Categorize by type (e.g., first-party vs. third-party). - let mut block_by_type = categorize_imports( - block, - src, - package, - known_modules, - known_local_folder, - extra_standard_library, - target_version, - ); + let mut block_by_type = categorize_imports(block, src, package, known_modules, target_version); let mut output = String::new(); @@ -422,8 +408,10 @@ mod tests { &Settings { isort: super::settings::Settings { known_modules: KnownModules::new( - ["foo.bar".to_string(), "baz".to_string()], - ["foo".to_string(), "__future__".to_string()], + vec!["foo.bar".to_string(), "baz".to_string()], + vec!["foo".to_string(), "__future__".to_string()], + vec![], + vec![], ), ..super::settings::Settings::default() }, @@ -442,7 +430,12 @@ mod tests { Path::new("isort").join(path).as_path(), &Settings { isort: super::settings::Settings { - known_modules: KnownModules::new(["foo".to_string()], ["foo.bar".to_string()]), + known_modules: KnownModules::new( + vec!["foo".to_string()], + vec!["foo.bar".to_string()], + vec![], + vec![], + ), ..super::settings::Settings::default() }, src: vec![test_resource_path("fixtures/isort")], @@ -478,7 +471,12 @@ mod tests { Path::new("isort").join(path).as_path(), &Settings { isort: super::settings::Settings { - known_local_folder: BTreeSet::from(["ruff".to_string()]), + known_modules: KnownModules::new( + vec![], + vec![], + vec!["ruff".to_string()], + vec![], + ), ..super::settings::Settings::default() }, src: vec![test_resource_path("fixtures/isort")], diff --git a/crates/ruff/src/rules/isort/rules/organize_imports.rs b/crates/ruff/src/rules/isort/rules/organize_imports.rs index 7e6cf5d27c12b..ddbde6ed69d88 100644 --- a/crates/ruff/src/rules/isort/rules/organize_imports.rs +++ b/crates/ruff/src/rules/isort/rules/organize_imports.rs @@ -122,13 +122,11 @@ pub fn organize_imports( &settings.src, package, settings.isort.combine_as_imports, - &settings.isort.extra_standard_library, settings.isort.force_single_line, settings.isort.force_sort_within_sections, settings.isort.force_wrap_aliases, &settings.isort.force_to_top, &settings.isort.known_modules, - &settings.isort.known_local_folder, settings.isort.order_by_type, settings.isort.relative_imports_order, &settings.isort.single_line_exclusions, diff --git a/crates/ruff/src/rules/isort/settings.rs b/crates/ruff/src/rules/isort/settings.rs index 8b78291ac0b48..c9acc603a6e1c 100644 --- a/crates/ruff/src/rules/isort/settings.rs +++ b/crates/ruff/src/rules/isort/settings.rs @@ -272,13 +272,11 @@ pub struct Options { pub struct Settings { pub required_imports: BTreeSet, pub combine_as_imports: bool, - pub extra_standard_library: BTreeSet, pub force_single_line: bool, pub force_sort_within_sections: bool, pub force_wrap_aliases: bool, pub force_to_top: BTreeSet, pub known_modules: KnownModules, - pub known_local_folder: BTreeSet, pub order_by_type: bool, pub relative_imports_order: RelativeImportsOrder, pub single_line_exclusions: BTreeSet, @@ -297,13 +295,11 @@ impl Default for Settings { Self { required_imports: BTreeSet::new(), combine_as_imports: false, - extra_standard_library: BTreeSet::new(), force_single_line: false, force_sort_within_sections: false, force_wrap_aliases: false, force_to_top: BTreeSet::new(), known_modules: KnownModules::default(), - known_local_folder: BTreeSet::new(), order_by_type: true, relative_imports_order: RelativeImportsOrder::default(), single_line_exclusions: BTreeSet::new(), @@ -324,9 +320,6 @@ impl From for Settings { Self { required_imports: BTreeSet::from_iter(options.required_imports.unwrap_or_default()), combine_as_imports: options.combine_as_imports.unwrap_or(false), - extra_standard_library: BTreeSet::from_iter( - options.extra_standard_library.unwrap_or_default(), - ), force_single_line: options.force_single_line.unwrap_or(false), force_sort_within_sections: options.force_sort_within_sections.unwrap_or(false), force_wrap_aliases: options.force_wrap_aliases.unwrap_or(false), @@ -334,8 +327,9 @@ impl From for Settings { known_modules: KnownModules::new( options.known_first_party.unwrap_or_default(), options.known_third_party.unwrap_or_default(), + options.known_local_folder.unwrap_or_default(), + options.extra_standard_library.unwrap_or_default(), ), - known_local_folder: BTreeSet::from_iter(options.known_local_folder.unwrap_or_default()), order_by_type: options.order_by_type.unwrap_or(true), relative_imports_order: options.relative_imports_order.unwrap_or_default(), single_line_exclusions: BTreeSet::from_iter( @@ -358,14 +352,20 @@ impl From for Options { Self { required_imports: Some(settings.required_imports.into_iter().collect()), combine_as_imports: Some(settings.combine_as_imports), - extra_standard_library: Some(settings.extra_standard_library.into_iter().collect()), + extra_standard_library: Some( + settings + .known_modules + .standard_library + .into_iter() + .collect(), + ), force_single_line: Some(settings.force_single_line), force_sort_within_sections: Some(settings.force_sort_within_sections), force_wrap_aliases: Some(settings.force_wrap_aliases), force_to_top: Some(settings.force_to_top.into_iter().collect()), known_first_party: Some(settings.known_modules.first_party.into_iter().collect()), known_third_party: Some(settings.known_modules.third_party.into_iter().collect()), - known_local_folder: Some(settings.known_local_folder.into_iter().collect()), + known_local_folder: Some(settings.known_modules.local_folder.into_iter().collect()), order_by_type: Some(settings.order_by_type), relative_imports_order: Some(settings.relative_imports_order), single_line_exclusions: Some(settings.single_line_exclusions.into_iter().collect()), From 92845ff3a3d77649381ff2e74e068567633a08f7 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 28 Mar 2023 23:47:33 -0400 Subject: [PATCH 3/3] Style changes --- crates/ruff/src/rules/isort/categorize.rs | 73 +++++++++++------------ 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/crates/ruff/src/rules/isort/categorize.rs b/crates/ruff/src/rules/isort/categorize.rs index ea34bc975a61d..602c4e9d9f93d 100644 --- a/crates/ruff/src/rules/isort/categorize.rs +++ b/crates/ruff/src/rules/isort/categorize.rs @@ -227,46 +227,45 @@ impl KnownModules { /// Return the [`ImportType`] for a given module, if it's been categorized as a known module /// by the user. fn categorize(&self, module_name: &str) -> Option<(ImportType, Reason)> { - // Happy path: no submodules, so we can check the module base and be done. - if !self.has_submodules { + if self.has_submodules { + // Check all module prefixes from the longest to the shortest (e.g., given + // `foo.bar.baz`, check `foo.bar.baz`, then `foo.bar`, then `foo`, taking the first, + // most precise match). + for i in module_name + .match_indices('.') + .map(|(i, _)| i) + .chain(iter::once(module_name.len())) + .rev() + { + let submodule = &module_name[0..i]; + if self.first_party.contains(submodule) { + return Some((ImportType::FirstParty, Reason::KnownFirstParty)); + } + if self.third_party.contains(submodule) { + return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); + } + if self.local_folder.contains(submodule) { + return Some((ImportType::LocalFolder, Reason::KnownLocalFolder)); + } + if self.standard_library.contains(submodule) { + return Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)); + } + } + None + } else { + // Happy path: no submodules, so we can check the module base and be done. let module_base = module_name.split('.').next().unwrap(); if self.first_party.contains(module_base) { - return Some((ImportType::FirstParty, Reason::KnownFirstParty)); - } - if self.third_party.contains(module_base) { - return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); - } - if self.local_folder.contains(module_base) { - return Some((ImportType::LocalFolder, Reason::KnownLocalFolder)); - } - if self.standard_library.contains(module_base) { - return Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)); + Some((ImportType::FirstParty, Reason::KnownFirstParty)) + } else if self.third_party.contains(module_base) { + Some((ImportType::ThirdParty, Reason::KnownThirdParty)) + } else if self.local_folder.contains(module_base) { + Some((ImportType::LocalFolder, Reason::KnownLocalFolder)) + } else if self.standard_library.contains(module_base) { + Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)) + } else { + None } } - - // Check all module prefixes from the longest to the shortest (e.g., given `foo.bar.baz`, - // check `foo.bar.baz`, then `foo.bar`, then `foo`, taking the first, most precise match). - for i in module_name - .match_indices('.') - .map(|(i, _)| i) - .chain(iter::once(module_name.len())) - .rev() - { - let submodule = &module_name[0..i]; - if self.first_party.contains(submodule) { - return Some((ImportType::FirstParty, Reason::KnownFirstParty)); - } - if self.third_party.contains(submodule) { - return Some((ImportType::ThirdParty, Reason::KnownThirdParty)); - } - if self.local_folder.contains(submodule) { - return Some((ImportType::LocalFolder, Reason::KnownLocalFolder)); - } - if self.standard_library.contains(submodule) { - return Some((ImportType::StandardLibrary, Reason::ExtraStandardLibrary)); - } - } - - None } }