Respect natural ordering for imports (#1374)

Author: charliermarsh

Cargo.lock

@@ -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" }

Cargo.toml

@@ -0,0 +1,16 @@
+import numpy1
+import numpy10
+import numpy2
+from numpy import (
+    cos,
+    int8,
+    sin,
+    int32,
+    int64,
+    tan,
+    uint8,
+    uint16,
+    int16,
+    uint32,
+    uint64,
+)

resources/test/fixtures/isort/natural_order.py

@@ -1,4 +1,4 @@
-use std::cmp::Reverse;
+use std::cmp::Ordering;
 use std::collections::{BTreeMap, BTreeSet};
 use std::path::{Path, PathBuf};
 
@@ -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,
@@ -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`.
@@ -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),
+                    }
+                })
             }),
     );
 
@@ -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"))]

src/isort/mod.rs

@@ -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
+

src/isort/snapshots/ruff__isort__tests__natural_order.py.snap

@@ -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)]
@@ -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)),
+        }
+    })
 }