Skip to content

Commit

Permalink
Respect natural ordering for imports (#1374)
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh authored Dec 25, 2022
1 parent 9bb470c commit ec80d1c
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 40 deletions.
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ ignore = { version = "0.4.18" }
itertools = { version = "0.10.5" }
libcst = { git = "https://github.com/charliermarsh/LibCST", rev = "f2f0b7a487a8725d161fe8b3ed73a6758b21e177" }
log = { version = "0.4.17" }
natord = { version = "1.0.9" }
nohash-hasher = { version = "0.2.0" }
notify = { version = "5.0.0" }
num-bigint = { version = "0.4.3" }
Expand Down
16 changes: 16 additions & 0 deletions resources/test/fixtures/isort/natural_order.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import numpy1
import numpy10
import numpy2
from numpy import (
cos,
int8,
sin,
int32,
int64,
tan,
uint8,
uint16,
int16,
uint32,
uint64,
)
31 changes: 14 additions & 17 deletions src/isort/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::cmp::Reverse;
use std::cmp::Ordering;
use std::collections::{BTreeMap, BTreeSet};
use std::path::{Path, PathBuf};

Expand All @@ -9,7 +9,7 @@ use rustpython_ast::{Stmt, StmtKind};

use crate::isort::categorize::{categorize, ImportType};
use crate::isort::comments::Comment;
use crate::isort::sorting::{member_key, module_key};
use crate::isort::sorting::{cmp_import_froms, cmp_members, cmp_modules};
use crate::isort::track::{Block, Trailer};
use crate::isort::types::{
AliasData, CommentSet, ImportBlock, ImportFromData, Importable, OrderedImportBlock,
Expand Down Expand Up @@ -379,7 +379,7 @@ fn sort_imports(block: ImportBlock) -> OrderedImportBlock {
block
.import
.into_iter()
.sorted_by_cached_key(|(alias, _)| module_key(alias.name, alias.asname)),
.sorted_by(|(alias1, _), (alias2, _)| cmp_modules(alias1, alias2)),
);

// Sort `StmtKind::ImportFrom`.
Expand Down Expand Up @@ -446,23 +446,19 @@ fn sort_imports(block: ImportBlock) -> OrderedImportBlock {
comments,
aliases
.into_iter()
.sorted_by_cached_key(|(alias, _)| member_key(alias.name, alias.asname))
.sorted_by(|(alias1, _), (alias2, _)| cmp_members(alias1, alias2))
.collect::<Vec<(AliasData, CommentSet)>>(),
)
})
.sorted_by_cached_key(|(import_from, _, aliases)| {
// Sort each `StmtKind::ImportFrom` by module key, breaking ties based on
// members.
(
Reverse(import_from.level),
import_from
.module
.as_ref()
.map(|module| module_key(module, None)),
aliases
.first()
.map(|(alias, _)| member_key(alias.name, alias.asname)),
)
.sorted_by(|(import_from1, _, aliases1), (import_from2, _, aliases2)| {
cmp_import_froms(import_from1, import_from2).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),
}
})
}),
);

Expand Down Expand Up @@ -569,6 +565,7 @@ mod tests {
#[test_case(Path::new("insert_empty_lines.py"))]
#[test_case(Path::new("insert_empty_lines.pyi"))]
#[test_case(Path::new("leading_prefix.py"))]
#[test_case(Path::new("natural_order.py"))]
#[test_case(Path::new("no_reorder_within_section.py"))]
#[test_case(Path::new("no_wrap_star.py"))]
#[test_case(Path::new("order_by_type.py"))]
Expand Down
20 changes: 20 additions & 0 deletions src/isort/snapshots/ruff__isort__tests__natural_order.py.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: src/isort/mod.rs
expression: checks
---
- kind: UnsortedImports
location:
row: 1
column: 0
end_location:
row: 17
column: 0
fix:
content: "import numpy1\nimport numpy2\nimport numpy10\nfrom numpy import (\n cos,\n int8,\n int16,\n int32,\n int64,\n sin,\n tan,\n uint8,\n uint16,\n uint32,\n uint64,\n)\n"
location:
row: 1
column: 0
end_location:
row: 17
column: 0

77 changes: 54 additions & 23 deletions src/isort/sorting.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
/// See: <https://github.com/PyCQA/isort/blob/12cc5fbd67eebf92eb2213b03c07b138ae1fb448/isort/sorting.py#L13>
use std::cmp::Ordering;

use crate::isort::types::{AliasData, ImportFromData};
use crate::python::string;

#[derive(PartialOrd, Ord, PartialEq, Eq)]
Expand All @@ -8,29 +11,57 @@ pub enum Prefix {
Variables,
}

pub fn module_key<'a>(
name: &'a str,
asname: Option<&'a String>,
) -> (String, &'a str, Option<&'a String>) {
(name.to_lowercase(), name, asname)
fn prefix(name: &str) -> Prefix {
if name.len() > 1 && string::is_upper(name) {
// Ex) `CONSTANT`
Prefix::Constants
} else if name.chars().next().map_or(false, char::is_uppercase) {
// Ex) `Class`
Prefix::Classes
} else {
// Ex) `variable`
Prefix::Variables
}
}

/// Compare two top-level modules.
pub fn cmp_modules(alias1: &AliasData, alias2: &AliasData) -> Ordering {
natord::compare_ignore_case(alias1.name, alias2.name)
.then_with(|| natord::compare(alias1.name, alias2.name))
.then_with(|| match (alias1.asname, alias2.asname) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(asname1), Some(asname2)) => natord::compare(asname1, asname2),
})
}

/// Compare two member imports within `StmtKind::ImportFrom` blocks.
pub fn cmp_members(alias1: &AliasData, alias2: &AliasData) -> Ordering {
prefix(alias1.name)
.cmp(&prefix(alias2.name))
.then_with(|| cmp_modules(alias1, alias2))
}

/// Compare two relative import levels.
pub fn cmp_levels(level1: Option<&usize>, level2: Option<&usize>) -> Ordering {
match (level1, level2) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(level1), Some(level2)) => level2.cmp(level1),
}
}

pub fn member_key<'a>(
name: &'a str,
asname: Option<&'a String>,
) -> (Prefix, String, Option<&'a String>) {
(
if name.len() > 1 && string::is_upper(name) {
// Ex) `CONSTANT`
Prefix::Constants
} else if name.chars().next().map_or(false, char::is_uppercase) {
// Ex) `Class`
Prefix::Classes
} else {
// Ex) `variable`
Prefix::Variables
},
name.to_lowercase(),
asname,
)
/// Compare two `StmtKind::ImportFrom` blocks.
pub fn cmp_import_froms(import_from1: &ImportFromData, import_from2: &ImportFromData) -> Ordering {
cmp_levels(import_from1.level, import_from2.level).then_with(|| {
match (&import_from1.module, import_from2.module) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(module1), Some(module2)) => natord::compare_ignore_case(module1, module2)
.then_with(|| natord::compare(module1, module2)),
}
})
}

0 comments on commit ec80d1c

Please sign in to comment.