From 558883299a75053ff4dc73118007f37141bfcbc0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 11 Nov 2022 22:41:39 -0500 Subject: [PATCH] Default to isort's import sort logic (#691) --- .../test/fixtures/isort/order_by_type.py | 12 ++ src/isort/mod.rs | 156 +++++++++++------- src/isort/plugins.rs | 4 +- .../ruff__isort__tests__order_by_type.py.snap | 22 +++ src/isort/sorting.rs | 34 ++++ src/isort/types.rs | 8 + src/pep8_naming/checks.rs | 15 +- src/pep8_naming/helpers.rs | 51 +----- src/python/mod.rs | 1 + src/python/string.rs | 50 ++++++ 10 files changed, 231 insertions(+), 122 deletions(-) create mode 100644 resources/test/fixtures/isort/order_by_type.py create mode 100644 src/isort/snapshots/ruff__isort__tests__order_by_type.py.snap create mode 100644 src/isort/sorting.rs create mode 100644 src/python/string.rs diff --git a/resources/test/fixtures/isort/order_by_type.py b/resources/test/fixtures/isort/order_by_type.py new file mode 100644 index 0000000000..836522d086 --- /dev/null +++ b/resources/test/fixtures/isort/order_by_type.py @@ -0,0 +1,12 @@ +import StringIO +import glob +import os +import shutil +import tempfile +import time +from subprocess import PIPE, Popen, STDOUT +from module import Class, CONSTANT, function, BASIC, Apple +import foo +import FOO +import BAR +import bar diff --git a/src/isort/mod.rs b/src/isort/mod.rs index e637bf6751..627532040b 100644 --- a/src/isort/mod.rs +++ b/src/isort/mod.rs @@ -1,15 +1,18 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::PathBuf; +use itertools::Itertools; use ropey::RopeBuilder; use rustpython_ast::{Stmt, StmtKind}; use crate::isort::categorize::{categorize, ImportType}; -use crate::isort::types::{AliasData, ImportBlock, ImportFromData, Importable}; +use crate::isort::sorting::{member_key, module_key}; +use crate::isort::types::{AliasData, ImportBlock, ImportFromData, Importable, OrderedImportBlock}; mod categorize; pub mod plugins; pub mod settings; +mod sorting; pub mod track; mod types; @@ -93,7 +96,37 @@ fn categorize_imports<'a>( block_by_type } -pub fn sort_imports( +fn sort_imports(block: ImportBlock) -> OrderedImportBlock { + let mut ordered: OrderedImportBlock = Default::default(); + // Sort `StmtKind::Import`. + for import in block + .import + .into_iter() + .sorted_by_cached_key(|alias| module_key(alias.name)) + { + ordered.import.push(import); + } + // Sort `StmtKind::ImportFrom`. + for (import_from, aliases) in + block + .import_from + .into_iter() + .sorted_by_cached_key(|(import_from, _)| { + import_from.module.as_ref().map(|module| module_key(module)) + }) + { + ordered.import_from.push(( + import_from, + aliases + .into_iter() + .sorted_by_cached_key(|alias| member_key(alias.name)) + .collect(), + )); + } + ordered +} + +pub fn format_imports( block: Vec<&Stmt>, line_length: &usize, src: &[PathBuf], @@ -116,46 +149,41 @@ pub fn sort_imports( // Generate replacement source code. let mut output = RopeBuilder::new(); let mut first_block = true; - for import_type in [ - ImportType::Future, - ImportType::StandardLibrary, - ImportType::ThirdParty, - ImportType::FirstParty, - ImportType::LocalFolder, - ] { - if let Some(import_block) = block_by_type.get(&import_type) { - // Add a blank line between every section. - if !first_block { - output.append("\n"); + for import_block in block_by_type.into_values() { + let import_block = sort_imports(import_block); + + // Add a blank line between every section. + if !first_block { + output.append("\n"); + } else { + first_block = false; + } + + // Format `StmtKind::Import` statements. + for AliasData { name, asname } in import_block.import.iter() { + if let Some(asname) = asname { + output.append(&format!("import {} as {}\n", name, asname)); } else { - first_block = false; + output.append(&format!("import {}\n", name)); } + } - // Format `StmtKind::Import` statements. - for AliasData { name, asname } in import_block.import.iter() { - if let Some(asname) = asname { - output.append(&format!("import {} as {}\n", name, asname)); - } else { - output.append(&format!("import {}\n", name)); - } - } + // Format `StmtKind::ImportFrom` statements. + for (import_from, aliases) in import_block.import_from.iter() { + let prelude: String = format!("from {} import ", import_from.module_name()); + let members: Vec = aliases + .iter() + .map(|AliasData { name, asname }| { + if let Some(asname) = asname { + format!("{} as {}", name, asname) + } else { + name.to_string() + } + }) + .collect(); - // Format `StmtKind::ImportFrom` statements. - for (import_from, aliases) in import_block.import_from.iter() { - let prelude: String = format!("from {} import ", import_from.module_name()); - let members: Vec = aliases - .iter() - .map(|AliasData { name, asname }| { - if let Some(asname) = asname { - format!("{} as {}", name, asname) - } else { - name.to_string() - } - }) - .collect(); - - // Can we fit the import on a single line? - let expected_len: usize = + // Can we fit the import on a single line? + let expected_len: usize = // `from base import ` prelude.len() // `member( as alias)?` @@ -163,36 +191,35 @@ pub fn sort_imports( // `, ` + 2 * (members.len() - 1); - if expected_len <= *line_length { - // `from base import ` - output.append(&prelude); - // `member( as alias)?(, )?` - for (index, part) in members.into_iter().enumerate() { - if index > 0 { - output.append(", "); - } - output.append(&part); + if expected_len <= *line_length { + // `from base import ` + output.append(&prelude); + // `member( as alias)?(, )?` + for (index, part) in members.into_iter().enumerate() { + if index > 0 { + output.append(", "); } - // `\n` - output.append("\n"); - } else { - // `from base import (\n` - output.append(&prelude); - output.append("("); - output.append("\n"); + output.append(&part); + } + // `\n` + output.append("\n"); + } else { + // `from base import (\n` + output.append(&prelude); + output.append("("); + output.append("\n"); - // ` member( as alias)?,\n` - for part in members { - output.append(INDENT); - output.append(&part); - output.append(","); - output.append("\n"); - } - - // `)\n` - output.append(")"); + // ` member( as alias)?,\n` + for part in members { + output.append(INDENT); + output.append(&part); + output.append(","); output.append("\n"); } + + // `)\n` + output.append(")"); + output.append("\n"); } } } @@ -217,6 +244,7 @@ mod tests { #[test_case(Path::new("import_from_after_import.py"))] #[test_case(Path::new("leading_prefix.py"))] #[test_case(Path::new("no_reorder_within_section.py"))] + #[test_case(Path::new("order_by_type.py"))] #[test_case(Path::new("preserve_indentation.py"))] #[test_case(Path::new("reorder_within_section.py"))] #[test_case(Path::new("separate_first_party_imports.py"))] diff --git a/src/isort/plugins.rs b/src/isort/plugins.rs index 968c183f9d..c8ea0b7e88 100644 --- a/src/isort/plugins.rs +++ b/src/isort/plugins.rs @@ -5,7 +5,7 @@ use crate::ast::types::Range; use crate::autofix::{fixer, Fix}; use crate::checks::CheckKind; use crate::docstrings::helpers::leading_space; -use crate::isort::sort_imports; +use crate::isort::format_imports; use crate::{Check, Settings, SourceCodeLocator}; fn extract_range(body: &[&Stmt]) -> Range { @@ -62,7 +62,7 @@ pub fn check_imports( let has_trailing_content = match_trailing_content(&body, locator); // Generate the sorted import block. - let expected = sort_imports( + let expected = format_imports( body, &settings.line_length, &settings.src, diff --git a/src/isort/snapshots/ruff__isort__tests__order_by_type.py.snap b/src/isort/snapshots/ruff__isort__tests__order_by_type.py.snap new file mode 100644 index 0000000000..b9e758c95b --- /dev/null +++ b/src/isort/snapshots/ruff__isort__tests__order_by_type.py.snap @@ -0,0 +1,22 @@ +--- +source: src/isort/mod.rs +expression: checks +--- +- kind: UnsortedImports + location: + row: 1 + column: 0 + end_location: + row: 13 + column: 0 + fix: + patch: + content: "import glob\nimport os\nimport shutil\nimport tempfile\nimport time\nfrom subprocess import PIPE, STDOUT, Popen\n\nimport BAR\nimport bar\nimport FOO\nimport foo\nimport StringIO\nfrom module import BASIC, CONSTANT, Apple, Class, function\n" + location: + row: 1 + column: 0 + end_location: + row: 13 + column: 0 + applied: false + diff --git a/src/isort/sorting.rs b/src/isort/sorting.rs new file mode 100644 index 0000000000..cf0c51753c --- /dev/null +++ b/src/isort/sorting.rs @@ -0,0 +1,34 @@ +/// See: https://github.com/PyCQA/isort/blob/12cc5fbd67eebf92eb2213b03c07b138ae1fb448/isort/sorting.py#L13 +use crate::python::string; + +#[derive(PartialOrd, Ord, PartialEq, Eq)] +pub enum Prefix { + Constants, + Classes, + Variables, +} + +pub fn module_key(module_name: &str) -> String { + module_name.to_lowercase() +} + +pub fn member_key(member_name: &str) -> (Prefix, String) { + ( + if member_name.len() > 1 && string::is_upper(member_name) { + // Ex) `CONSTANT` + Prefix::Constants + } else if member_name + .chars() + .next() + .map(|char| char.is_uppercase()) + .unwrap_or(false) + { + // Ex) `Class` + Prefix::Classes + } else { + // Ex) `variable` + Prefix::Variables + }, + member_name.to_lowercase(), + ) +} diff --git a/src/isort/types.rs b/src/isort/types.rs index 042126755b..4958882068 100644 --- a/src/isort/types.rs +++ b/src/isort/types.rs @@ -53,3 +53,11 @@ pub struct ImportBlock<'a> { // Set of (name, asname). pub import: BTreeSet>, } + +#[derive(Debug, Default)] +pub struct OrderedImportBlock<'a> { + // Map from (module, level) to `AliasData`. + pub import_from: Vec<(ImportFromData<'a>, Vec>)>, + // Set of (name, asname). + pub import: Vec>, +} diff --git a/src/pep8_naming/checks.rs b/src/pep8_naming/checks.rs index 4772bfa796..524966061d 100644 --- a/src/pep8_naming/checks.rs +++ b/src/pep8_naming/checks.rs @@ -5,6 +5,7 @@ use crate::checks::{Check, CheckKind}; use crate::pep8_naming::helpers; use crate::pep8_naming::helpers::FunctionType; use crate::pep8_naming::settings::Settings; +use crate::python::string; /// N801 pub fn invalid_class_name(class_def: &Stmt, name: &str) -> Option { @@ -133,7 +134,7 @@ pub fn constant_imported_as_non_constant( name: &str, asname: &str, ) -> Option { - if helpers::is_upper(name) && !helpers::is_upper(asname) { + if string::is_upper(name) && !string::is_upper(asname) { return Some(Check::new( CheckKind::ConstantImportedAsNonConstant(name.to_string(), asname.to_string()), Range::from_located(import_from), @@ -148,7 +149,7 @@ pub fn lowercase_imported_as_non_lowercase( name: &str, asname: &str, ) -> Option { - if !helpers::is_upper(name) && helpers::is_lower(name) && asname.to_lowercase() != asname { + if !string::is_upper(name) && string::is_lower(name) && asname.to_lowercase() != asname { return Some(Check::new( CheckKind::LowercaseImportedAsNonLowercase(name.to_string(), asname.to_string()), Range::from_located(import_from), @@ -163,7 +164,7 @@ pub fn camelcase_imported_as_lowercase( name: &str, asname: &str, ) -> Option { - if helpers::is_camelcase(name) && helpers::is_lower(asname) { + if helpers::is_camelcase(name) && string::is_lower(asname) { return Some(Check::new( CheckKind::CamelcaseImportedAsLowercase(name.to_string(), asname.to_string()), Range::from_located(import_from), @@ -179,8 +180,8 @@ pub fn camelcase_imported_as_constant( asname: &str, ) -> Option { if helpers::is_camelcase(name) - && !helpers::is_lower(asname) - && helpers::is_upper(asname) + && !string::is_lower(asname) + && string::is_upper(asname) && !helpers::is_acronym(name, asname) { return Some(Check::new( @@ -230,8 +231,8 @@ pub fn camelcase_imported_as_acronym( asname: &str, ) -> Option { if helpers::is_camelcase(name) - && !helpers::is_lower(asname) - && helpers::is_upper(asname) + && !string::is_lower(asname) + && string::is_upper(asname) && helpers::is_acronym(name, asname) { return Some(Check::new( diff --git a/src/pep8_naming/helpers.rs b/src/pep8_naming/helpers.rs index d337186fd9..36bf664a7e 100644 --- a/src/pep8_naming/helpers.rs +++ b/src/pep8_naming/helpers.rs @@ -4,6 +4,7 @@ use rustpython_ast::{Expr, ExprKind}; use crate::ast::helpers::match_name_or_attr; use crate::ast::types::{Scope, ScopeKind}; use crate::pep8_naming::settings::Settings; +use crate::python::string::{is_lower, is_upper}; const CLASS_METHODS: [&str; 3] = ["__new__", "__init_subclass__", "__class_getitem__"]; const METACLASS_BASES: [&str; 2] = ["type", "ABCMeta"]; @@ -59,30 +60,6 @@ pub fn function_type( } } -pub fn is_lower(s: &str) -> bool { - let mut cased = false; - for c in s.chars() { - if c.is_uppercase() { - return false; - } else if !cased && c.is_lowercase() { - cased = true; - } - } - cased -} - -pub fn is_upper(s: &str) -> bool { - let mut cased = false; - for c in s.chars() { - if c.is_lowercase() { - return false; - } else if !cased && c.is_uppercase() { - cased = true; - } - } - cased -} - pub fn is_camelcase(name: &str) -> bool { !is_lower(name) && !is_upper(name) && !name.contains('_') } @@ -103,31 +80,7 @@ pub fn is_acronym(name: &str, asname: &str) -> bool { #[cfg(test)] mod tests { - use crate::pep8_naming::helpers::{ - is_acronym, is_camelcase, is_lower, is_mixed_case, is_upper, - }; - - #[test] - fn test_is_lower() -> () { - assert!(is_lower("abc")); - assert!(is_lower("a_b_c")); - assert!(is_lower("a2c")); - assert!(!is_lower("aBc")); - assert!(!is_lower("ABC")); - assert!(!is_lower("")); - assert!(!is_lower("_")); - } - - #[test] - fn test_is_upper() -> () { - assert!(is_upper("ABC")); - assert!(is_upper("A_B_C")); - assert!(is_upper("A2C")); - assert!(!is_upper("aBc")); - assert!(!is_upper("abc")); - assert!(!is_upper("")); - assert!(!is_upper("_")); - } + use crate::pep8_naming::helpers::{is_acronym, is_camelcase, is_mixed_case}; #[test] fn test_is_camelcase() -> () { diff --git a/src/python/mod.rs b/src/python/mod.rs index 7381cc6810..f609cb82c7 100644 --- a/src/python/mod.rs +++ b/src/python/mod.rs @@ -1,5 +1,6 @@ pub mod builtins; pub mod future; pub mod keyword; +pub mod string; pub mod sys; pub mod typing; diff --git a/src/python/string.rs b/src/python/string.rs new file mode 100644 index 0000000000..50be2861da --- /dev/null +++ b/src/python/string.rs @@ -0,0 +1,50 @@ +pub fn is_lower(s: &str) -> bool { + let mut cased = false; + for c in s.chars() { + if c.is_uppercase() { + return false; + } else if !cased && c.is_lowercase() { + cased = true; + } + } + cased +} + +pub fn is_upper(s: &str) -> bool { + let mut cased = false; + for c in s.chars() { + if c.is_lowercase() { + return false; + } else if !cased && c.is_uppercase() { + cased = true; + } + } + cased +} + +#[cfg(test)] +mod tests { + use crate::python::string::{is_lower, is_upper}; + + #[test] + fn test_is_lower() -> () { + assert!(is_lower("abc")); + assert!(is_lower("a_b_c")); + assert!(is_lower("a2c")); + assert!(!is_lower("aBc")); + assert!(!is_lower("ABC")); + assert!(!is_lower("")); + assert!(!is_lower("_")); + } + + #[test] + fn test_is_upper() -> () { + assert!(is_upper("ABC")); + assert!(is_upper("A_B_C")); + assert!(is_upper("A2C")); + assert!(!is_upper("aBc")); + assert!(!is_upper("abc")); + assert!(!is_upper("")); + assert!(!is_upper("_")); + } +}