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 :)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.209` -> `^0.0.210` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.210/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.210/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.210/compatibility-slim/0.0.209)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.210/confidence-slim/0.0.209)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.210`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.210) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.209...v0.0.210) #### What's Changed - Do not Change Quotation Style for `PT006` Autofix by [@saadmk11](https://togithub.com/saadmk11) in [astral-sh/ruff#1600 - Implement autofix for `PT022` by [@harupy](https://togithub.com/harupy) in [astral-sh/ruff#1604 - Add isort.order-by-type boolean setting by [@mattoberle](https://togithub.com/mattoberle) in [astral-sh/ruff#1607 - Fix \*arg and \*\*kwarg handling for Google docstrings by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1608 - Associate inline comments with parenthesized `ImportFrom` statements by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1609 - Fix leftover whitespace when removing `pass` for `PIE790` by [@edgarrmondragon](https://togithub.com/edgarrmondragon) in [astral-sh/ruff#1612 - Treat .pyi files as **future** annotations-enabled by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1616 - Treat convention as setting ignore, rather than select by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1611 - Avoid byte-string conversions by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1618 - Implement missing fixes for `PT006` by [@edgarrmondragon](https://togithub.com/edgarrmondragon) in [astral-sh/ruff#1622 - Implement `yield`-to-`yield from` conversion by [@colin99d](https://togithub.com/colin99d) in [astral-sh/ruff#1544 - Add some more users to the README by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1623 - Check `SIM118` in comprehension by [@harupy](https://togithub.com/harupy) in [astral-sh/ruff#1627 #### New Contributors - [@mattoberle](https://togithub.com/mattoberle) made their first contribution in [astral-sh/ruff#1607 **Full Changelog**: astral-sh/ruff@v0.0.209...v0.0.210 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

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

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

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

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