From ee7d445ef5966f2cef12bbe6bbb12824e13408fa Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 29 Oct 2023 21:48:28 -0700 Subject: [PATCH] Use associated methods for `MemberKey` and `ModuleKey` (#8337) --- crates/ruff_linter/src/rules/isort/mod.rs | 12 +- crates/ruff_linter/src/rules/isort/order.rs | 8 +- crates/ruff_linter/src/rules/isort/sorting.rs | 147 +++++++++--------- 3 files changed, 87 insertions(+), 80 deletions(-) diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index 8c909046fb..e82700398f 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -2,20 +2,21 @@ use std::path::{Path, PathBuf}; +use itertools::Itertools; + use annotate::annotate_imports; use block::{Block, Trailer}; pub(crate) use categorize::categorize; use categorize::categorize_imports; pub use categorize::{ImportSection, ImportType}; use comments::Comment; -use itertools::Itertools; use normalize::normalize_imports; use order::order_imports; use ruff_python_ast::PySourceType; use ruff_python_codegen::Stylist; use ruff_source_file::Locator; use settings::Settings; -use sorting::module_key; +use sorting::ModuleKey; use types::EitherImport::{Import, ImportFrom}; use types::{AliasData, EitherImport, ImportBlock, TrailingComma}; @@ -183,9 +184,9 @@ fn format_import_block( imports .sorted_by_cached_key(|import| match import { Import((alias, _)) => { - module_key(Some(alias.name), alias.asname, None, None, settings) + ModuleKey::from_module(Some(alias.name), alias.asname, None, None, settings) } - ImportFrom((import_from, _, _, aliases)) => module_key( + ImportFrom((import_from, _, _, aliases)) => ModuleKey::from_module( import_from.module, None, import_from.level, @@ -260,10 +261,11 @@ mod tests { use std::path::Path; use anyhow::Result; - use ruff_text_size::Ranged; use rustc_hash::FxHashMap; use test_case::test_case; + use ruff_text_size::Ranged; + use crate::assert_messages; use crate::registry::Rule; use crate::rules::isort::categorize::{ImportSection, KnownModules}; diff --git a/crates/ruff_linter/src/rules/isort/order.rs b/crates/ruff_linter/src/rules/isort/order.rs index 19710b0587..1437bc38c0 100644 --- a/crates/ruff_linter/src/rules/isort/order.rs +++ b/crates/ruff_linter/src/rules/isort/order.rs @@ -1,7 +1,7 @@ use itertools::Itertools; use super::settings::Settings; -use super::sorting::{member_key, module_key}; +use super::sorting::{MemberKey, ModuleKey}; use super::types::{AliasData, CommentSet, ImportBlock, ImportFromStatement, OrderedImportBlock}; pub(crate) fn order_imports<'a>( @@ -14,7 +14,7 @@ pub(crate) fn order_imports<'a>( ordered .import .extend(block.import.into_iter().sorted_by_cached_key(|(alias, _)| { - module_key(Some(alias.name), alias.asname, None, None, settings) + ModuleKey::from_module(Some(alias.name), alias.asname, None, None, settings) })); // Sort `Stmt::ImportFrom`. @@ -51,14 +51,14 @@ pub(crate) fn order_imports<'a>( aliases .into_iter() .sorted_by_cached_key(|(alias, _)| { - member_key(alias.name, alias.asname, settings) + MemberKey::from_member(alias.name, alias.asname, settings) }) .collect::>(), ) }, ) .sorted_by_cached_key(|(import_from, _, _, aliases)| { - module_key( + ModuleKey::from_module( import_from.module, None, import_from.level, diff --git a/crates/ruff_linter/src/rules/isort/sorting.rs b/crates/ruff_linter/src/rules/isort/sorting.rs index 2f6bdefc24..5b082b0dab 100644 --- a/crates/ruff_linter/src/rules/isort/sorting.rs +++ b/crates/ruff_linter/src/rules/isort/sorting.rs @@ -1,7 +1,8 @@ -/// See: +//! See: + +use std::{borrow::Cow, cmp::Ordering, cmp::Reverse}; + use natord; -use std::cmp::Reverse; -use std::{borrow::Cow, cmp::Ordering}; use ruff_python_stdlib::str; @@ -63,84 +64,88 @@ impl<'a> From for NatOrdStr<'a> { } } -#[derive(Debug, PartialOrd, Ord, PartialEq, Eq, Copy, Clone)] +#[derive(Debug, PartialOrd, Ord, PartialEq, Eq)] pub(crate) enum Distance { Nearest(u32), Furthest(Reverse), } -type ModuleKey<'a> = ( - Distance, - Option, - Option>, - Option>, - Option>, - Option>, -); - -/// Returns a comparable key to capture the desired sorting order for an imported module (e.g., +/// A comparable key to capture the desired sorting order for an imported module (e.g., /// `foo` in `from foo import bar`). -pub(crate) fn module_key<'a>( - name: Option<&'a str>, - asname: Option<&'a str>, - level: Option, - first_alias: Option<(&'a str, Option<&'a str>)>, - settings: &Settings, -) -> ModuleKey<'a> { - let distance = match settings.relative_imports_order { - RelativeImportsOrder::ClosestToFurthest => Distance::Nearest(level.unwrap_or_default()), - RelativeImportsOrder::FurthestToClosest => { - Distance::Furthest(Reverse(level.unwrap_or_default())) - } - }; - let force_to_top = name.map(|name| !settings.force_to_top.contains(name)); // `false` < `true` so we get forced to top first - let maybe_lowercase_name = name - .and_then(|name| (!settings.case_sensitive).then_some(NatOrdStr(maybe_lowercase(name)))); - let module_name = name.map(NatOrdStr::from); - let asname = asname.map(NatOrdStr::from); - let first_alias = first_alias.map(|(name, asname)| member_key(name, asname, settings)); - - ( - distance, - force_to_top, - maybe_lowercase_name, - module_name, - asname, - first_alias, - ) +#[derive(Debug, PartialOrd, Ord, PartialEq, Eq)] +pub(crate) struct ModuleKey<'a> { + distance: Distance, + force_to_top: Option, + maybe_lowercase_name: Option>, + module_name: Option>, + asname: Option>, + first_alias: Option>, } -type MemberKey<'a> = ( - bool, - Option, - Option>, - NatOrdStr<'a>, - Option>, -); +impl<'a> ModuleKey<'a> { + pub(crate) fn from_module( + name: Option<&'a str>, + asname: Option<&'a str>, + level: Option, + first_alias: Option<(&'a str, Option<&'a str>)>, + settings: &Settings, + ) -> Self { + let distance = match settings.relative_imports_order { + RelativeImportsOrder::ClosestToFurthest => Distance::Nearest(level.unwrap_or_default()), + RelativeImportsOrder::FurthestToClosest => { + Distance::Furthest(Reverse(level.unwrap_or_default())) + } + }; + let force_to_top = name.map(|name| !settings.force_to_top.contains(name)); // `false` < `true` so we get forced to top first + let maybe_lowercase_name = name.and_then(|name| { + (!settings.case_sensitive).then_some(NatOrdStr(maybe_lowercase(name))) + }); + let module_name = name.map(NatOrdStr::from); + let asname = asname.map(NatOrdStr::from); + let first_alias = + first_alias.map(|(name, asname)| MemberKey::from_member(name, asname, settings)); -/// Returns a comparable key to capture the desired sorting order for an imported member (e.g., -/// `bar` in `from foo import bar`). -pub(crate) fn member_key<'a>( - name: &'a str, - asname: Option<&'a str>, - settings: &Settings, -) -> MemberKey<'a> { - let not_star_import = name != "*"; // `false` < `true` so we get star imports first - let member_type = settings - .order_by_type - .then_some(member_type(name, settings)); - let maybe_lowercase_name = - (!settings.case_sensitive).then_some(NatOrdStr(maybe_lowercase(name))); - let module_name = NatOrdStr::from(name); - let asname = asname.map(NatOrdStr::from); + Self { + distance, + force_to_top, + maybe_lowercase_name, + module_name, + asname, + first_alias, + } + } +} - ( - not_star_import, - member_type, - maybe_lowercase_name, - module_name, - asname, - ) +/// A comparable key to capture the desired sorting order for an imported member (e.g., `bar` in +/// `from foo import bar`). +#[derive(Debug, PartialOrd, Ord, PartialEq, Eq)] +pub(crate) struct MemberKey<'a> { + not_star_import: bool, + member_type: Option, + maybe_lowercase_name: Option>, + module_name: NatOrdStr<'a>, + asname: Option>, +} + +impl<'a> MemberKey<'a> { + pub(crate) fn from_member(name: &'a str, asname: Option<&'a str>, settings: &Settings) -> Self { + let not_star_import = name != "*"; // `false` < `true` so we get star imports first + let member_type = settings + .order_by_type + .then_some(member_type(name, settings)); + let maybe_lowercase_name = + (!settings.case_sensitive).then_some(NatOrdStr(maybe_lowercase(name))); + let module_name = NatOrdStr::from(name); + let asname = asname.map(NatOrdStr::from); + + Self { + not_star_import, + member_type, + maybe_lowercase_name, + module_name, + asname, + } + } } /// Lowercase the given string, if it contains any uppercase characters.