Add isort.order-by-type boolean setting #1607

mattoberle · 2023-01-03T22:08:39Z

The default isort behavior is order-by-type = true. When imports are ordered by type:

CONSTANT_VARIABLES are first.
CamelCaseClasses are second.
everything_else is third.

https://pycqa.github.io/isort/docs/configuration/options.html#order-by-type

When order-by-type = false imports are ordered alphabetically (case-insensitive).

eg.

order-by-type = false

import BAR
import bar
import FOO
import foo
import StringIO
from module import Apple, BASIC, Class, CONSTANT, function

order-by-type = true

import BAR
import bar
import FOO
import foo
import StringIO
from module import BASIC, CONSTANT, Apple, Class, function

This PR is what I could hobble together with my limited rust.
In particular, I couldn't get a default option value of true without impl Default for Settings and wasn't sure the best way to get member_key aware of the setting without quite a bit of argument passing.

Open to feedback on anything in the PR and if this is an isort feature that would have a home in ruff!

charliermarsh

Seems reasonable to me! You'll need to run cargo +nightly dev generate-all to update the JSON schema, README, etc.

Thanks for getting involved!

charliermarsh · 2023-01-03T22:11:22Z

src/isort/settings.rs

@@ -99,9 +111,23 @@ impl Settings {
            force_wrap_aliases: options.force_wrap_aliases.unwrap_or_default(),
            known_first_party: BTreeSet::from_iter(options.known_first_party.unwrap_or_default()),
            known_third_party: BTreeSet::from_iter(options.known_third_party.unwrap_or_default()),
+            order_by_type: options.order_by_type.unwrap_or_default(),


I think this should be options.order_by_type.unwrap_or(true). As-is, it will default to false.

charliermarsh · 2023-01-03T22:12:14Z

src/isort/sorting.rs

@@ -18,9 +18,12 @@ pub fn module_key<'a>(
 pub fn member_key<'a>(
    name: &'a str,
    asname: Option<&'a String>,
+    order_by_type: bool,
 ) -> (Prefix, String, Option<&'a String>) {


Hmm, maybe return (Option<Prefix>, ...), and then if !order_by_type { None } else if name.len() > 1 && string::is_upper(name) { Some(Prefix::Constants) } else if ...?

Oops, it looks like there my branch was out-of-date, sorry about that!
Just rebased- member_key is gone and the responsibilities are split between cmp_members and prefix.

All good, I should've noticed too, I wrote that newer code 😂

The default `isort` behavior is `order-by-type = true`. When imports are ordered by type: 1. `CONSTANT_VARIABLES` are first. 2. `CamelCaseClasses` are second. 3. `everything_else` is third. - https://pycqa.github.io/isort/docs/configuration/options.html#order-by-type When `order-by-type = false` imports are ordered alphabetically (case-insensitive). eg. `order-by-type = false` ```py import BAR import bar import FOO import foo import StringIO from module import Apple, BASIC, Class, CONSTANT, function ``` `order-by-type = true` ```py import BAR import bar import FOO import foo import StringIO from module import BASIC, CONSTANT, Apple, Class, function ```

mattoberle · 2023-01-03T22:39:09Z

src/isort/sorting.rs

+pub fn cmp_members(alias1: &AliasData, alias2: &AliasData, order_by_type: bool) -> Ordering {
+    if order_by_type {
+        prefix(alias1.name)
+            .cmp(&prefix(alias2.name))
+            .then_with(|| cmp_modules(alias1, alias2))
+    } else {
+        cmp_modules(alias1, alias2)
+    }


Maybe a bit weird that order_by_type essentially turns this into a different existing function.
Is it better to select which function is used somewhere in mod.rs?

I think this is ok. The current setup is a bit confusing as-is anyway (mostly just the nomenclature isn't that clear), but I like that this at least encapsulates the member sorting in one place.

charliermarsh · 2023-01-03T23:08:03Z

Thanks for this! It's much appreciated. Hope you'll consider sticking around :)

charliermarsh reviewed Jan 3, 2023

View reviewed changes

mattoberle force-pushed the isort/order-by-type branch from f7722b9 to 86b17f7 Compare January 3, 2023 22:22

Remove order_by_type as argument to prefix

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

12d25be

mattoberle commented Jan 3, 2023

View reviewed changes

charliermarsh merged commit 03275c9 into astral-sh:main Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isort.order-by-type boolean setting #1607

Add isort.order-by-type boolean setting #1607

mattoberle commented Jan 3, 2023

charliermarsh left a comment

charliermarsh Jan 3, 2023

charliermarsh Jan 3, 2023

mattoberle Jan 3, 2023 •

edited

Loading

charliermarsh Jan 3, 2023

mattoberle Jan 3, 2023

charliermarsh Jan 3, 2023

charliermarsh commented Jan 3, 2023

Add isort.order-by-type boolean setting #1607

Add isort.order-by-type boolean setting #1607

Conversation

mattoberle commented Jan 3, 2023

charliermarsh left a comment

Choose a reason for hiding this comment

charliermarsh Jan 3, 2023

Choose a reason for hiding this comment

charliermarsh Jan 3, 2023

Choose a reason for hiding this comment

mattoberle Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jan 3, 2023

Choose a reason for hiding this comment

mattoberle Jan 3, 2023

Choose a reason for hiding this comment

charliermarsh Jan 3, 2023

Choose a reason for hiding this comment

charliermarsh commented Jan 3, 2023

mattoberle Jan 3, 2023 •

edited

Loading