From ee533332ed7f73d961c10a30c034b272ccdfc871 Mon Sep 17 00:00:00 2001 From: bluthej Date: Tue, 26 Sep 2023 19:17:18 +0200 Subject: [PATCH] Refactor: use `Settings` struct (#7665) ## Summary Pass around a `Settings` struct instead of individual members to simplify function signatures and to make it easier to add new settings. This PR was suggested in [this comment](https://github.com/astral-sh/ruff/issues/1567#issuecomment-1734182803). ## Note on the choices I chose which functions to modify based on which seem most likely to use new settings, but suggestions on my choices are welcome! --- crates/ruff_linter/src/rules/isort/mod.rs | 109 ++++-------------- .../ruff_linter/src/rules/isort/normalize.rs | 23 ++-- crates/ruff_linter/src/rules/isort/order.rs | 61 +++------- .../src/rules/isort/rules/organize_imports.rs | 21 +--- crates/ruff_linter/src/rules/isort/sorting.rs | 96 ++++++--------- 5 files changed, 80 insertions(+), 230 deletions(-) diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index 84b0cf7c42..bbe155c6ef 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -1,6 +1,5 @@ //! Rules from [isort](https://pypi.org/project/isort/). -use std::collections::BTreeSet; use std::path::{Path, PathBuf}; use annotate::annotate_imports; @@ -14,14 +13,12 @@ use order::order_imports; use ruff_python_ast::PySourceType; use ruff_python_codegen::Stylist; use ruff_source_file::Locator; -use settings::RelativeImportsOrder; +use settings::Settings; use sorting::cmp_either_import; use types::EitherImport::{Import, ImportFrom}; -use types::{AliasData, EitherImport, TrailingComma}; +use types::{AliasData, EitherImport, ImportBlock, TrailingComma}; use crate::line_width::{LineLength, LineWidthBuilder}; -use crate::rules::isort::categorize::KnownModules; -use crate::rules::isort::types::ImportBlock; use crate::settings::types::PythonVersion; mod annotate; @@ -74,49 +71,25 @@ pub(crate) fn format_imports( src: &[PathBuf], package: Option<&Path>, source_type: PySourceType, - combine_as_imports: bool, - force_single_line: bool, - force_sort_within_sections: bool, - case_sensitive: bool, - force_wrap_aliases: bool, - force_to_top: &BTreeSet, - known_modules: &KnownModules, - order_by_type: bool, - detect_same_package: bool, - relative_imports_order: RelativeImportsOrder, - single_line_exclusions: &BTreeSet, - split_on_trailing_comma: bool, - classes: &BTreeSet, - constants: &BTreeSet, - variables: &BTreeSet, - no_lines_before: &BTreeSet, - lines_after_imports: isize, - lines_between_types: usize, - forced_separate: &[String], target_version: PythonVersion, - section_order: &[ImportSection], + settings: &Settings, ) -> String { let trailer = &block.trailer; let block = annotate_imports( &block.imports, comments, locator, - split_on_trailing_comma, + settings.split_on_trailing_comma, source_type, ); // Normalize imports (i.e., deduplicate, aggregate `from` imports). - let block = normalize_imports( - block, - combine_as_imports, - force_single_line, - single_line_exclusions, - ); + let block = normalize_imports(block, settings); // Categorize imports. let mut output = String::new(); - for block in split::split_by_forced_separate(block, forced_separate) { + for block in split::split_by_forced_separate(block, &settings.forced_separate) { let block_output = format_import_block( block, line_length, @@ -124,22 +97,8 @@ pub(crate) fn format_imports( stylist, src, package, - force_sort_within_sections, - case_sensitive, - force_wrap_aliases, - force_to_top, - known_modules, - order_by_type, - detect_same_package, - relative_imports_order, - split_on_trailing_comma, - classes, - constants, - variables, - no_lines_before, - lines_between_types, target_version, - section_order, + settings, ); if !block_output.is_empty() && !output.is_empty() { @@ -150,6 +109,7 @@ pub(crate) fn format_imports( output.push_str(block_output.as_str()); } + let lines_after_imports = settings.lines_after_imports; match trailer { None => {} Some(Trailer::Sibling) => { @@ -183,30 +143,16 @@ fn format_import_block( stylist: &Stylist, src: &[PathBuf], package: Option<&Path>, - force_sort_within_sections: bool, - case_sensitive: bool, - force_wrap_aliases: bool, - force_to_top: &BTreeSet, - known_modules: &KnownModules, - order_by_type: bool, - detect_same_package: bool, - relative_imports_order: RelativeImportsOrder, - split_on_trailing_comma: bool, - classes: &BTreeSet, - constants: &BTreeSet, - variables: &BTreeSet, - no_lines_before: &BTreeSet, - lines_between_types: usize, target_version: PythonVersion, - section_order: &[ImportSection], + settings: &Settings, ) -> String { // Categorize by type (e.g., first-party vs. third-party). let mut block_by_type = categorize_imports( block, src, package, - detect_same_package, - known_modules, + settings.detect_same_package, + &settings.known_modules, target_version, ); @@ -215,26 +161,17 @@ fn format_import_block( // Generate replacement source code. let mut is_first_block = true; let mut pending_lines_before = false; - for import_section in section_order { + for import_section in &settings.section_order { let import_block = block_by_type.remove(import_section); - if !no_lines_before.contains(import_section) { + if !settings.no_lines_before.contains(import_section) { pending_lines_before = true; } let Some(import_block) = import_block else { continue; }; - let imports = order_imports( - import_block, - order_by_type, - case_sensitive, - relative_imports_order, - classes, - constants, - variables, - force_to_top, - ); + let imports = order_imports(import_block, settings); let imports = { let mut imports = imports @@ -243,16 +180,8 @@ fn format_import_block( .map(Import) .chain(imports.import_from.into_iter().map(ImportFrom)) .collect::>(); - if force_sort_within_sections { - imports.sort_by(|import1, import2| { - cmp_either_import( - import1, - import2, - relative_imports_order, - force_to_top, - case_sensitive, - ) - }); + if settings.force_sort_within_sections { + imports.sort_by(|import1, import2| cmp_either_import(import1, import2, settings)); }; imports }; @@ -269,6 +198,7 @@ fn format_import_block( let mut lines_inserted = false; let mut has_direct_import = false; let mut is_first_statement = true; + let lines_between_types = settings.lines_between_types; for import in imports { match import { Import((alias, comments)) => { @@ -299,9 +229,10 @@ fn format_import_block( line_length, indentation_width, stylist, - force_wrap_aliases, + settings.force_wrap_aliases, is_first_statement, - split_on_trailing_comma && matches!(trailing_comma, TrailingComma::Present), + settings.split_on_trailing_comma + && matches!(trailing_comma, TrailingComma::Present), )); } } diff --git a/crates/ruff_linter/src/rules/isort/normalize.rs b/crates/ruff_linter/src/rules/isort/normalize.rs index ceee1cb4a3..0189658023 100644 --- a/crates/ruff_linter/src/rules/isort/normalize.rs +++ b/crates/ruff_linter/src/rules/isort/normalize.rs @@ -1,15 +1,10 @@ -use std::collections::BTreeSet; - -use crate::rules::isort::types::TrailingComma; - -use super::types::{AliasData, ImportBlock, ImportFromData}; +use super::settings::Settings; +use super::types::{AliasData, ImportBlock, ImportFromData, TrailingComma}; use super::AnnotatedImport; pub(crate) fn normalize_imports<'a>( imports: Vec>, - combine_as_imports: bool, - force_single_line: bool, - single_line_exclusions: &'a BTreeSet, + settings: &Settings, ) -> ImportBlock<'a> { let mut block = ImportBlock::default(); for import in imports { @@ -56,8 +51,10 @@ pub(crate) fn normalize_imports<'a>( trailing_comma, } => { // Whether to track each member of the import as a separate entry. - let isolate_aliases = force_single_line - && module.map_or(true, |module| !single_line_exclusions.contains(module)) + let isolate_aliases = settings.force_single_line + && module.map_or(true, |module| { + !settings.single_line_exclusions.contains(module) + }) && !names.first().is_some_and(|alias| alias.name == "*"); // Insert comments on the statement itself. @@ -95,7 +92,7 @@ pub(crate) fn normalize_imports<'a>( .import_from_star .entry(ImportFromData { module, level }) .or_default() - } else if alias.asname.is_none() || combine_as_imports { + } else if alias.asname.is_none() || settings.combine_as_imports { block .import_from .entry(ImportFromData { module, level }) @@ -130,7 +127,9 @@ pub(crate) fn normalize_imports<'a>( .import_from_star .entry(ImportFromData { module, level }) .or_default() - } else if !isolate_aliases && (alias.asname.is_none() || combine_as_imports) { + } else if !isolate_aliases + && (alias.asname.is_none() || settings.combine_as_imports) + { block .import_from .entry(ImportFromData { module, level }) diff --git a/crates/ruff_linter/src/rules/isort/order.rs b/crates/ruff_linter/src/rules/isort/order.rs index a2210ed43a..c2110457a0 100644 --- a/crates/ruff_linter/src/rules/isort/order.rs +++ b/crates/ruff_linter/src/rules/isort/order.rs @@ -1,24 +1,14 @@ use std::cmp::Ordering; -use std::collections::BTreeSet; use itertools::Itertools; -use crate::rules::isort::types::ImportFromStatement; - -use super::settings::RelativeImportsOrder; +use super::settings::Settings; use super::sorting::{cmp_import_from, cmp_members, cmp_modules}; -use super::types::{AliasData, CommentSet, ImportBlock, OrderedImportBlock}; +use super::types::{AliasData, CommentSet, ImportBlock, ImportFromStatement, OrderedImportBlock}; -#[allow(clippy::too_many_arguments)] pub(crate) fn order_imports<'a>( block: ImportBlock<'a>, - order_by_type: bool, - case_sensitive: bool, - relative_imports_order: RelativeImportsOrder, - classes: &'a BTreeSet, - constants: &'a BTreeSet, - variables: &'a BTreeSet, - force_to_top: &'a BTreeSet, + settings: &Settings, ) -> OrderedImportBlock<'a> { let mut ordered = OrderedImportBlock::default(); @@ -27,9 +17,7 @@ pub(crate) fn order_imports<'a>( block .import .into_iter() - .sorted_by(|(alias1, _), (alias2, _)| { - cmp_modules(alias1, alias2, force_to_top, case_sensitive) - }), + .sorted_by(|(alias1, _), (alias2, _)| cmp_modules(alias1, alias2, settings)), ); // Sort `Stmt::ImportFrom`. @@ -66,16 +54,7 @@ pub(crate) fn order_imports<'a>( aliases .into_iter() .sorted_by(|(alias1, _), (alias2, _)| { - cmp_members( - alias1, - alias2, - order_by_type, - classes, - constants, - variables, - force_to_top, - case_sensitive, - ) + cmp_members(alias1, alias2, settings) }) .collect::>(), ) @@ -83,27 +62,15 @@ pub(crate) fn order_imports<'a>( ) .sorted_by( |(import_from1, _, _, aliases1), (import_from2, _, _, aliases2)| { - cmp_import_from( - import_from1, - import_from2, - relative_imports_order, - force_to_top, - case_sensitive, - ) - .then_with(|| match (aliases1.first(), aliases2.first()) { - (None, None) => Ordering::Equal, - (None, Some(_)) => Ordering::Less, - (Some(_), None) => Ordering::Greater, - (Some((alias1, _)), Some((alias2, _))) => cmp_members( - alias1, - alias2, - order_by_type, - classes, - constants, - variables, - force_to_top, - case_sensitive, - ), + cmp_import_from(import_from1, import_from2, settings).then_with(|| { + match (aliases1.first(), aliases2.first()) { + (None, None) => Ordering::Equal, + (None, Some(_)) => Ordering::Less, + (Some(_), None) => Ordering::Greater, + (Some((alias1, _)), Some((alias2, _))) => { + cmp_members(alias1, alias2, settings) + } + } }) }, ), diff --git a/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs b/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs index 62b2043f8c..e778c9b120 100644 --- a/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs +++ b/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs @@ -126,27 +126,8 @@ pub(crate) fn organize_imports( &settings.src, package, source_type, - settings.isort.combine_as_imports, - settings.isort.force_single_line, - settings.isort.force_sort_within_sections, - settings.isort.case_sensitive, - settings.isort.force_wrap_aliases, - &settings.isort.force_to_top, - &settings.isort.known_modules, - settings.isort.order_by_type, - settings.isort.detect_same_package, - settings.isort.relative_imports_order, - &settings.isort.single_line_exclusions, - settings.isort.split_on_trailing_comma, - &settings.isort.classes, - &settings.isort.constants, - &settings.isort.variables, - &settings.isort.no_lines_before, - settings.isort.lines_after_imports, - settings.isort.lines_between_types, - &settings.isort.forced_separate, settings.target_version, - &settings.isort.section_order, + &settings.isort, ); // Expand the span the entire range, including leading and trailing space. diff --git a/crates/ruff_linter/src/rules/isort/sorting.rs b/crates/ruff_linter/src/rules/isort/sorting.rs index 444e550406..959d5f7a13 100644 --- a/crates/ruff_linter/src/rules/isort/sorting.rs +++ b/crates/ruff_linter/src/rules/isort/sorting.rs @@ -4,11 +4,9 @@ use std::collections::BTreeSet; use ruff_python_stdlib::str; -use crate::rules::isort::types::Importable; - -use super::settings::RelativeImportsOrder; +use super::settings::{RelativeImportsOrder, Settings}; use super::types::EitherImport::{Import, ImportFrom}; -use super::types::{AliasData, EitherImport, ImportFromData}; +use super::types::{AliasData, EitherImport, ImportFromData, Importable}; #[derive(PartialOrd, Ord, PartialEq, Eq, Copy, Clone)] pub(crate) enum Prefix { @@ -17,19 +15,14 @@ pub(crate) enum Prefix { Variables, } -fn prefix( - name: &str, - classes: &BTreeSet, - constants: &BTreeSet, - variables: &BTreeSet, -) -> Prefix { - if constants.contains(name) { +fn prefix(name: &str, settings: &Settings) -> Prefix { + if settings.constants.contains(name) { // Ex) `CONSTANT` Prefix::Constants - } else if classes.contains(name) { + } else if settings.classes.contains(name) { // Ex) `CLASS` Prefix::Classes - } else if variables.contains(name) { + } else if settings.variables.contains(name) { // Ex) `variable` Prefix::Variables } else if name.len() > 1 && str::is_cased_uppercase(name) { @@ -52,15 +45,10 @@ fn cmp_force_to_top(name1: &str, name2: &str, force_to_top: &BTreeSet) - } /// Compare two top-level modules. -pub(crate) fn cmp_modules( - alias1: &AliasData, - alias2: &AliasData, - force_to_top: &BTreeSet, - case_sensitive: bool, -) -> Ordering { - cmp_force_to_top(alias1.name, alias2.name, force_to_top) +pub(crate) fn cmp_modules(alias1: &AliasData, alias2: &AliasData, settings: &Settings) -> Ordering { + cmp_force_to_top(alias1.name, alias2.name, &settings.force_to_top) .then_with(|| { - if case_sensitive { + if settings.case_sensitive { natord::compare(alias1.name, alias2.name) } else { natord::compare_ignore_case(alias1.name, alias2.name) @@ -76,27 +64,17 @@ pub(crate) fn cmp_modules( } /// Compare two member imports within `Stmt::ImportFrom` blocks. -#[allow(clippy::too_many_arguments)] -pub(crate) fn cmp_members( - alias1: &AliasData, - alias2: &AliasData, - order_by_type: bool, - classes: &BTreeSet, - constants: &BTreeSet, - variables: &BTreeSet, - force_to_top: &BTreeSet, - case_sensitive: bool, -) -> Ordering { +pub(crate) fn cmp_members(alias1: &AliasData, alias2: &AliasData, settings: &Settings) -> Ordering { match (alias1.name == "*", alias2.name == "*") { (true, false) => Ordering::Less, (false, true) => Ordering::Greater, _ => { - if order_by_type { - prefix(alias1.name, classes, constants, variables) - .cmp(&prefix(alias2.name, classes, constants, variables)) - .then_with(|| cmp_modules(alias1, alias2, force_to_top, case_sensitive)) + if settings.order_by_type { + prefix(alias1.name, settings) + .cmp(&prefix(alias2.name, settings)) + .then_with(|| cmp_modules(alias1, alias2, settings)) } else { - cmp_modules(alias1, alias2, force_to_top, case_sensitive) + cmp_modules(alias1, alias2, settings) } } } @@ -123,20 +101,18 @@ pub(crate) fn cmp_levels( pub(crate) fn cmp_import_from( import_from1: &ImportFromData, import_from2: &ImportFromData, - relative_imports_order: RelativeImportsOrder, - force_to_top: &BTreeSet, - case_sensitive: bool, + settings: &Settings, ) -> Ordering { cmp_levels( import_from1.level, import_from2.level, - relative_imports_order, + settings.relative_imports_order, ) .then_with(|| { cmp_force_to_top( &import_from1.module_name(), &import_from2.module_name(), - force_to_top, + &settings.force_to_top, ) }) .then_with(|| match (&import_from1.module, import_from2.module) { @@ -144,7 +120,7 @@ pub(crate) fn cmp_import_from( (None, Some(_)) => Ordering::Less, (Some(_), None) => Ordering::Greater, (Some(module1), Some(module2)) => { - if case_sensitive { + if settings.case_sensitive { natord::compare(module1, module2) } else { natord::compare_ignore_case(module1, module2) @@ -157,11 +133,15 @@ pub(crate) fn cmp_import_from( fn cmp_import_import_from( import: &AliasData, import_from: &ImportFromData, - force_to_top: &BTreeSet, - case_sensitive: bool, + settings: &Settings, ) -> Ordering { - cmp_force_to_top(import.name, &import_from.module_name(), force_to_top).then_with(|| { - if case_sensitive { + cmp_force_to_top( + import.name, + &import_from.module_name(), + &settings.force_to_top, + ) + .then_with(|| { + if settings.case_sensitive { natord::compare(import.name, import_from.module.unwrap_or_default()) } else { natord::compare_ignore_case(import.name, import_from.module.unwrap_or_default()) @@ -174,26 +154,18 @@ fn cmp_import_import_from( pub(crate) fn cmp_either_import( a: &EitherImport, b: &EitherImport, - relative_imports_order: RelativeImportsOrder, - force_to_top: &BTreeSet, - case_sensitive: bool, + settings: &Settings, ) -> Ordering { match (a, b) { - (Import((alias1, _)), Import((alias2, _))) => { - cmp_modules(alias1, alias2, force_to_top, case_sensitive) - } + (Import((alias1, _)), Import((alias2, _))) => cmp_modules(alias1, alias2, settings), (ImportFrom((import_from, ..)), Import((alias, _))) => { - cmp_import_import_from(alias, import_from, force_to_top, case_sensitive).reverse() + cmp_import_import_from(alias, import_from, settings).reverse() } (Import((alias, _)), ImportFrom((import_from, ..))) => { - cmp_import_import_from(alias, import_from, force_to_top, case_sensitive) + cmp_import_import_from(alias, import_from, settings) + } + (ImportFrom((import_from1, ..)), ImportFrom((import_from2, ..))) => { + cmp_import_from(import_from1, import_from2, settings) } - (ImportFrom((import_from1, ..)), ImportFrom((import_from2, ..))) => cmp_import_from( - import_from1, - import_from2, - relative_imports_order, - force_to_top, - case_sensitive, - ), } }