From c9931a548ff07c031130571f3664343bea224026 Mon Sep 17 00:00:00 2001 From: Michael Merickel Date: Thu, 29 Feb 2024 20:32:03 -0700 Subject: [PATCH] Implement isort's `default-section` setting (#10149) ## Summary This fixes https://github.com/astral-sh/ruff/issues/7868. Support isort's `default-section` feature which allows any imports that match sections that are not in `section-order` to be mapped to a specifically named section. https://pycqa.github.io/isort/docs/configuration/options.html#default-section This has a few implications: - It is no longer required that all known sections are defined in `section-order`. - This is technically a bw-incompat change because currently if folks define custom groups, and do not define a `section-order`, the code used to add all known sections to `section-order` while emitting warnings. **However, when this happened, users would be seeing warnings so I do not think it should count as a bw-incompat change.** ## Test Plan - Added a new test. - Did not break any existing tests. Finally, I ran the following config against Pyramid's complex codebase that was previously using isort and this change worked there. ### pyramid's previous isort config https://github.com/Pylons/pyramid/blob/5f7e286b0629b0a5f1225fe51172cba77eb8fda1/pyproject.toml#L22-L37 ```toml [tool.isort] profile = "black" multi_line_output = 3 src_paths = ["src", "tests"] skip_glob = ["docs/*"] include_trailing_comma = true force_grid_wrap = false combine_as_imports = true line_length = 79 force_sort_within_sections = true no_lines_before = "THIRDPARTY" sections = "FUTURE,THIRDPARTY,FIRSTPARTY,LOCALFOLDER" default_section = "THIRDPARTY" known_first_party = "pyramid" ``` ### tested with ruff isort config ```toml [tool.ruff.lint.isort] case-sensitive = true combine-as-imports = true force-sort-within-sections = true section-order = [ "future", "third-party", "first-party", "local-folder", ] default-section = "third-party" known-first-party = [ "pyramid", ] ``` --- ...ow_settings__display_default_settings.snap | 1 + .../isort/default_section_user_defined.py | 9 +++ .../fixtures/isort/no_standard_library.py | 9 +++ .../rules/typing_only_runtime_import.rs | 2 + .../ruff_linter/src/rules/isort/categorize.rs | 25 ++++++-- crates/ruff_linter/src/rules/isort/mod.rs | 61 +++++++++++++++++++ .../ruff_linter/src/rules/isort/settings.rs | 3 + ...ction_default_section_user_defined.py.snap | 4 ++ ...andard_library_no_standard_library.py.snap | 29 +++++++++ crates/ruff_workspace/src/options.rs | 43 +++++++------ ruff.schema.json | 11 ++++ 11 files changed, 174 insertions(+), 23 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/isort/default_section_user_defined.py create mode 100644 crates/ruff_linter/resources/test/fixtures/isort/no_standard_library.py create mode 100644 crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__default_section_can_map_to_user_defined_section_default_section_user_defined.py.snap create mode 100644 crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__no_standard_library_no_standard_library.py.snap diff --git a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index d86ffbac62a55..6b7d064333d18 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -311,6 +311,7 @@ linter.isort.section_order = [ known { type = first_party }, known { type = local_folder }, ] +linter.isort.default_section = known { type = third_party } linter.isort.no_sections = false linter.isort.from_first = false linter.isort.length_sort = false diff --git a/crates/ruff_linter/resources/test/fixtures/isort/default_section_user_defined.py b/crates/ruff_linter/resources/test/fixtures/isort/default_section_user_defined.py new file mode 100644 index 0000000000000..50d9218d9cfb1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/isort/default_section_user_defined.py @@ -0,0 +1,9 @@ +from __future__ import annotations + +import django.settings +from library import foo +import os +import pytz +import sys + +from . import local diff --git a/crates/ruff_linter/resources/test/fixtures/isort/no_standard_library.py b/crates/ruff_linter/resources/test/fixtures/isort/no_standard_library.py new file mode 100644 index 0000000000000..73f88ce067a99 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/isort/no_standard_library.py @@ -0,0 +1,9 @@ +from __future__ import annotations + +import os +import django.settings +from library import foo +import pytz + +from . import local +import sys diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 17812c89989cc..8b12e0086a719 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -304,6 +304,8 @@ pub(crate) fn typing_only_runtime_import( &checker.settings.isort.known_modules, checker.settings.target_version, checker.settings.isort.no_sections, + &checker.settings.isort.section_order, + &checker.settings.isort.default_section, ) { ImportSection::Known(ImportType::LocalFolder | ImportType::FirstParty) => { ImportType::FirstParty diff --git a/crates/ruff_linter/src/rules/isort/categorize.rs b/crates/ruff_linter/src/rules/isort/categorize.rs index c00640ea12ac2..f7fe77dd71138 100644 --- a/crates/ruff_linter/src/rules/isort/categorize.rs +++ b/crates/ruff_linter/src/rules/isort/categorize.rs @@ -84,6 +84,7 @@ enum Reason<'a> { NoMatch, UserDefinedSection, NoSections, + DisabledSection(&'a ImportSection), } #[allow(clippy::too_many_arguments)] @@ -96,9 +97,11 @@ pub(crate) fn categorize<'a>( known_modules: &'a KnownModules, target_version: PythonVersion, no_sections: bool, + section_order: &'a [ImportSection], + default_section: &'a ImportSection, ) -> &'a ImportSection { let module_base = module_name.split('.').next().unwrap(); - let (import_type, reason) = { + let (mut import_type, mut reason) = { if matches!(level, None | Some(0)) && module_base == "__future__" { (&ImportSection::Known(ImportType::Future), Reason::Future) } else if no_sections { @@ -134,12 +137,14 @@ pub(crate) fn categorize<'a>( Reason::KnownFirstParty, ) } else { - ( - &ImportSection::Known(ImportType::ThirdParty), - Reason::NoMatch, - ) + (default_section, Reason::NoMatch) } }; + // If a value is not in `section_order` then map it to `default_section`. + if !section_order.contains(import_type) { + reason = Reason::DisabledSection(import_type); + import_type = default_section; + } debug!( "Categorized '{}' as {:?} ({:?})", module_name, import_type, reason @@ -176,6 +181,8 @@ pub(crate) fn categorize_imports<'a>( known_modules: &'a KnownModules, target_version: PythonVersion, no_sections: bool, + section_order: &'a [ImportSection], + default_section: &'a ImportSection, ) -> BTreeMap<&'a ImportSection, ImportBlock<'a>> { let mut block_by_type: BTreeMap<&ImportSection, ImportBlock> = BTreeMap::default(); // Categorize `Stmt::Import`. @@ -189,6 +196,8 @@ pub(crate) fn categorize_imports<'a>( known_modules, target_version, no_sections, + section_order, + default_section, ); block_by_type .entry(import_type) @@ -207,6 +216,8 @@ pub(crate) fn categorize_imports<'a>( known_modules, target_version, no_sections, + section_order, + default_section, ); block_by_type .entry(classification) @@ -225,6 +236,8 @@ pub(crate) fn categorize_imports<'a>( known_modules, target_version, no_sections, + section_order, + default_section, ); block_by_type .entry(classification) @@ -243,6 +256,8 @@ pub(crate) fn categorize_imports<'a>( known_modules, target_version, no_sections, + section_order, + default_section, ); block_by_type .entry(classification) diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index ddaf33cc27180..47ec3c4dc9335 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -173,6 +173,8 @@ fn format_import_block( &settings.known_modules, target_version, settings.no_sections, + &settings.section_order, + &settings.default_section, ); let mut output = String::new(); @@ -934,6 +936,65 @@ mod tests { Ok(()) } + #[test_case(Path::new("default_section_user_defined.py"))] + fn default_section_can_map_to_user_defined_section(path: &Path) -> Result<()> { + let snapshot = format!( + "default_section_can_map_to_user_defined_section_{}", + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &LinterSettings { + isort: super::settings::Settings { + known_modules: KnownModules::new( + vec![], + vec![], + vec![], + vec![], + FxHashMap::from_iter([("django".to_string(), vec![pattern("django")])]), + ), + section_order: vec![ + ImportSection::Known(ImportType::Future), + ImportSection::UserDefined("django".to_string()), + ImportSection::Known(ImportType::FirstParty), + ImportSection::Known(ImportType::LocalFolder), + ], + force_sort_within_sections: true, + default_section: ImportSection::UserDefined("django".to_string()), + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..LinterSettings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + #[test_case(Path::new("no_standard_library.py"))] + fn no_standard_library(path: &Path) -> Result<()> { + let snapshot = format!("no_standard_library_{}", path.to_string_lossy()); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &LinterSettings { + isort: super::settings::Settings { + section_order: vec![ + ImportSection::Known(ImportType::Future), + ImportSection::Known(ImportType::ThirdParty), + ImportSection::Known(ImportType::FirstParty), + ImportSection::Known(ImportType::LocalFolder), + ], + force_sort_within_sections: true, + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..LinterSettings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Path::new("no_lines_before.py"))] fn no_lines_before(path: &Path) -> Result<()> { let snapshot = format!("no_lines_before.py_{}", path.to_string_lossy()); diff --git a/crates/ruff_linter/src/rules/isort/settings.rs b/crates/ruff_linter/src/rules/isort/settings.rs index b5b13363b4c4a..f86d593a75968 100644 --- a/crates/ruff_linter/src/rules/isort/settings.rs +++ b/crates/ruff_linter/src/rules/isort/settings.rs @@ -67,6 +67,7 @@ pub struct Settings { pub lines_between_types: usize, pub forced_separate: Vec, pub section_order: Vec, + pub default_section: ImportSection, pub no_sections: bool, pub from_first: bool, pub length_sort: bool, @@ -97,6 +98,7 @@ impl Default for Settings { lines_between_types: 0, forced_separate: Vec::new(), section_order: ImportType::iter().map(ImportSection::Known).collect(), + default_section: ImportSection::Known(ImportType::ThirdParty), no_sections: false, from_first: false, length_sort: false, @@ -132,6 +134,7 @@ impl Display for Settings { self.lines_between_types, self.forced_separate | array, self.section_order | array, + self.default_section, self.no_sections, self.from_first, self.length_sort, diff --git a/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__default_section_can_map_to_user_defined_section_default_section_user_defined.py.snap b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__default_section_can_map_to_user_defined_section_default_section_user_defined.py.snap new file mode 100644 index 0000000000000..ed369f0fd61f0 --- /dev/null +++ b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__default_section_can_map_to_user_defined_section_default_section_user_defined.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/isort/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__no_standard_library_no_standard_library.py.snap b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__no_standard_library_no_standard_library.py.snap new file mode 100644 index 0000000000000..8dfcfe5183d0b --- /dev/null +++ b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__no_standard_library_no_standard_library.py.snap @@ -0,0 +1,29 @@ +--- +source: crates/ruff_linter/src/rules/isort/mod.rs +--- +no_standard_library.py:1:1: I001 [*] Import block is un-sorted or un-formatted + | +1 | / from __future__ import annotations +2 | | +3 | | import os +4 | | import django.settings +5 | | from library import foo +6 | | import pytz +7 | | +8 | | from . import local +9 | | import sys + | + = help: Organize imports + +ℹ Safe fix +1 1 | from __future__ import annotations +2 2 | +3 |-import os +4 3 | import django.settings +5 4 | from library import foo + 5 |+import os +6 6 | import pytz + 7 |+import sys +7 8 | +8 9 | from . import local +9 |-import sys diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 2a648093dc7bf..4d049e344e0ee 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2099,6 +2099,16 @@ pub struct IsortOptions { )] pub section_order: Option>, + /// Define a default section for any imports that don't fit into the specified `section-order`. + #[option( + default = r#"third-party"#, + value_type = "str", + example = r#" + default-section = "third-party" + "# + )] + pub default_section: Option, + /// Put all imports into the same section bucket. /// /// For example, rather than separating standard library and third-party imports, as in: @@ -2226,6 +2236,9 @@ impl IsortOptions { if no_sections && self.section_order.is_some() { warn_user_once!("`section-order` is ignored when `no-sections` is set to `true`"); } + if no_sections && self.default_section.is_some() { + warn_user_once!("`default-section` is ignored when `no-sections` is set to `true`"); + } if no_sections && self.sections.is_some() { warn_user_once!("`sections` is ignored when `no-sections` is set to `true`"); } @@ -2241,6 +2254,10 @@ impl IsortOptions { let mut section_order: Vec<_> = self .section_order .unwrap_or_else(|| ImportType::iter().map(ImportSection::Known).collect()); + let default_section = self + .default_section + .unwrap_or(ImportSection::Known(ImportType::ThirdParty)); + let known_first_party = self .known_first_party .map(|names| { @@ -2344,24 +2361,13 @@ impl IsortOptions { } } - // Add all built-in sections to `section_order`, if not already present. - for section in ImportType::iter().map(ImportSection::Known) { - if !section_order.contains(§ion) { - warn_user_once!( - "`section-order` is missing built-in section: `{:?}`", - section - ); - section_order.push(section); - } - } - - // Add all user-defined sections to `section-order`, if not already present. - for section_name in sections.keys() { - let section = ImportSection::UserDefined(section_name.clone()); - if !section_order.contains(§ion) { - warn_user_once!("`section-order` is missing section: `{:?}`", section); - section_order.push(section); - } + // Verify that `default_section` is in `section_order`. + if !section_order.contains(&default_section) { + warn_user_once!( + "`section-order` must contain `default-section`: {:?}", + default_section, + ); + section_order.push(default_section.clone()); } Ok(isort::settings::Settings { @@ -2394,6 +2400,7 @@ impl IsortOptions { lines_between_types, forced_separate: Vec::from_iter(self.forced_separate.unwrap_or_default()), section_order, + default_section, no_sections, from_first, length_sort: self.length_sort.unwrap_or(false), diff --git a/ruff.schema.json b/ruff.schema.json index 5f756e68e6011..a415b966c0512 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1474,6 +1474,17 @@ "type": "string" } }, + "default-section": { + "description": "Define a default section for any imports that don't fit into the specified `section-order`.", + "anyOf": [ + { + "$ref": "#/definitions/ImportSection" + }, + { + "type": "null" + } + ] + }, "detect-same-package": { "description": "Whether to automatically mark imports from within the same package as first-party. For example, when `detect-same-package = true`, then when analyzing files within the `foo` package, any imports from within the `foo` package will be considered first-party.\n\nThis heuristic is often unnecessary when `src` is configured to detect all first-party sources; however, if `src` is _not_ configured, this heuristic can be useful to detect first-party imports from _within_ (but not _across_) first-party packages.", "type": [