Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement isort custom sections and ordering #2419

Closed
ngnpope opened this issue Jan 31, 2023 · 30 comments
Closed

Implement isort custom sections and ordering #2419

ngnpope opened this issue Jan 31, 2023 · 30 comments
Labels
isort Related to import sorting

Comments

@ngnpope
Copy link
Contributor

ngnpope commented Jan 31, 2023

isort has support for configuring custom sections and ordering.

This support would also need to be tied in with no-lines-before for the custom sections.

I would imagine that the following isort configuration:

known_django=django
known_pandas=pandas,numpy
sections=FUTURE,STDLIB,DJANGO,THIRDPARTY,PANDAS,FIRSTPARTY,LOCALFOLDER
no_lines_before=DJANGO,LOCALFOLDER

Would become the following in ruff:

[tool.ruff.isort]
no-lines-before = ["django", "local-folder"]
section-order = ["future", "standard-library", "django", "third-party", "pandas", "first-party", "local-folder"]

[tool.ruff.isort.sections]
django = ["django"]
pandas = ["pandas", "numpy"]
@charliermarsh charliermarsh added the isort Related to import sorting label Jan 31, 2023
@spaceone
Copy link
Contributor

spaceone commented Feb 1, 2023

Support for known-local-folder would be nice.

@charliermarsh
Copy link
Member

Isn't local-folder always denoted by a relative import? What does known-local-folder do?

@spaceone
Copy link
Contributor

spaceone commented Feb 1, 2023

local-folder is the name of the section/category while known-local-folder are the modules/packages which should be treated for the local-folder section.

e.g.:

sections=FUTURE,STDLIB,FIRSTPARTY,THIRDPARTY,LOCALFOLDER
known_local_folder=myproject,test,foo

it has nothing to do with relative imports.

@g-as
Copy link
Contributor

g-as commented Feb 1, 2023

Aren't you mixing known_first_party and known_local_folder?

https://pycqa.github.io/isort/docs/configuration/options.html#known-first-party

https://pycqa.github.io/isort/docs/configuration/options.html#known-local-folder

Force isort to recognize a module as being a local folder. Generally, this is reserved for relative imports (from . import module).

@charliermarsh
Copy link
Member

I guess isort supports including non-relative imports in local folder? Strikes me as unusual!

@spaceone
Copy link
Contributor

spaceone commented Feb 8, 2023

Aren't you mixing known_first_party and known_local_folder?

Kind of yes, but we use this to separate our "known first party" modules from some special modules (some only for "testing" and one which can only be imported in a special C application): We use it as visual separation from other modules.

I guess isort supports including non-relative imports in local folder? Strikes me as unusual!
yes, I think the implementation of it is just a regular "section", so no special behavior.

→ So for me it's not important if I just add another section which I name by myself or use the standard isort section "known-local-folder".

spaceone added a commit to spaceone/ruff that referenced this issue Feb 8, 2023
spaceone added a commit to spaceone/ruff that referenced this issue Feb 8, 2023
spaceone added a commit to spaceone/ruff that referenced this issue Feb 10, 2023
spaceone added a commit to spaceone/ruff that referenced this issue Feb 10, 2023
@QuentinN42
Copy link

Hey, I'll try to implement the [tool.ruff.isort.sections] feature :)

@QuentinN42
Copy link

After reading the code, I think I first need to pass an MR about allowing section order.

If I understand right the code in the PyCQA/isort GH, they return a str in the place module to be able to order it dynamically in the place module.

@charliermarsh today we use an enum ImportType to order all imports :

https://github.com/charliermarsh/ruff/blob/36d134fd41a8378f1c3f7bb69a7053ecefb17cff/crates/ruff/src/rules/isort/categorize.rs#L14-L24

What do you think about changing that to a BTreeMap or an HashMap to allow runtime update of it (custom categorize) ?

@charliermarsh
Copy link
Member

I wish I had some way to assess the importance or popularity of this functionality, since it does introduce a bunch of complexity. Does anyone want to make a strong case for it?

(I personally tend not to use any of isort's configuration options, but I've been open to adding them over time as other's have put together PRs, since they generally haven't introduced too much complexity. Though I may end up regretting supporting those over time, we'll see :))

(Having not looked at the code much...) if we do go forward with this, I might suggest adding a UserDefined(String) variant to ImportType, rather than changing to to a stringy-typed map. Or even treating that as an entirely separate struct, like:

pub enum Section {
  KnownType(ImportType),
  UserDefined(String),
}

Since we're dealing with two orthogonal concepts: whether a module is first-part or third-party etc., vs. whether it should be categorized in a certain section.

@Redoubts
Copy link

I suspect this is going to be popular in the dark matter that is closed-source business code.

But I think it’s also good if you have something like

‘’’
from my_special_loader import a_module
import a_module.submodule
‘’’
isort would reverse the order without the ability to configure sections.

@Atilla
Copy link

Atilla commented Feb 28, 2023

This might not be a very helpful contribution, but:

I am following this because I want to replace our closed-source business code repo linters with ruff. We have a bunch of those isort rules, mostly to split django-related 3rd party stuff from the rest of it, as far as I can observe.

People looked at the diff without this on, and I'm pretty sure I could convince them to just stop using that grouping, some even prefer it. But it's a very large diff in the repo :)

@charliermarsh
Copy link
Member

No, that's very helpful! I'm just trying to build some intuition for how important it is, so any data points are welcome (this goes for all commenters and onlookers :)).

@mastacheata
Copy link

mastacheata commented Feb 28, 2023

We have the django stuff as a separate section cause there's usually a lot of that and having that grouped together just looks "nicer".

Obviously there's no technical need for changing the order or the spacing for such a block, it's just a convention we have in our repo of about 900 files which would be changed should we replace isort with the corresponding selector in ruff.

I guess the same can be argued for pandas as that's the other example in the isort docs.

Just noticed, we could pretty much achieve the same thing by adding django to force-separate, now this adds it to the bottom of the list behind our local packages.

Here's an example of how one of our typical viewsets in DRF looks like: (click to expand)
import csv

from django.db.models import Case, CharField, F, Q, When
from django.http import HttpResponse
from django.shortcuts import get_object_or_404
from django.utils import timezone
from django.utils.timezone import now

from rest_framework import status
from rest_framework.decorators import action
from rest_framework.response import Response
from rest_framework.serializers import Serializer
from rest_framework_serializer_extensions.views import SerializerExtensionsAPIViewMixin

from empto.company.api.filters import CertificateFilter
from empto.company.api.permissions import (
    ChangeCompanyPermission,
    ExportCertificatesPermission,
    ListCertificatePermission,
    ListMyCertificatePermission,
    ReviewCertificatePermission,
)
from empto.company.api.serializers import CertificateListSerializer, CertificateReviewSerializer, CertificateSerializer
from empto.company.models import Certificate, Company
from empto.company.services import CertificateCSVExportService, CompanyCertificateEmailService
from empto.core.api.pagination import StandardResultsSetPagination
from empto.core.api.permissions import EmptoDefaultPermissions
from empto.core.api.views import NoDeleteModelViewSet, UserSpecificQuerySetMixin
from empto.dsz.models import DszContract
from empto.notification.models import Notification, NotificationSubscription

@QuentinN42
Copy link

It's particulary usefull in the monorepo / big projects :
You quickly get like 30 lines of imports...

Being able to create some groups like :

  • Datascience : numpy, pandas ...
  • Software: redis, pika ...
  • Core: my_s3, my_logger ...
  • Metier: my_worker, scheduler ...

Transform this :

import numpy
import my_s3
import my_logger
import my_worker
import pandas
import pika
import redis
import scheduler

Into this :

import numpy
import pandas

import pika
import redis

import my_s3
import my_logger

import my_worker
import scheduler

@QuentinN42
Copy link

But yes I'ts not required at all as a priority.
I think we can take some time to think about the best way of implementing it here without copying 1 to 1 isort features.
I will read the release notes and issues of this feature on the isort repo and place a summary here so we have some base work material.

@charliermarsh
Copy link
Member

Seems like there's enough demand + concrete use-cases for this to at least be worth exploring, and scoping out how much complexity it actually adds to the implementation.

@g-as
Copy link
Contributor

g-as commented Mar 2, 2023

My 2c:

one of my use cases, that has not been mentioned yet, is adding a test deps section to isolate test tooling imports in test files:

known_tests_deps = ["factory", "faker", "pytest", "pytest_django", "respx", "testfixtures", "time_machine"]

@QuentinN42
Copy link

Here is the first MRs in the isort repo : PyCQA/isort#275

I don't find any discussions about the implementation.

Maybe we want to define some sections in a list : thx like

[[tool.ruff.import-section]]
name="my_section_name"
line-before = true  # or no-lines-before
packages = [ "pandas", "..." ]

Then we can order section with a list of string.

Or we can add a level (int) in this section that allow us to get rid of this section order string.

PS : this is not a real proposition, I just want to put here as most ideas to be able to take the better decision

WeilerP added a commit to WeilerP/tradeSeq-py that referenced this issue Mar 15, 2023
As of now, `ruff` does not allow custom sorting and ordering of imports.
See astral-sh/ruff#2419.
WeilerP added a commit to WeilerP/tradeSeq-py that referenced this issue Mar 15, 2023
As of now, `ruff` does not allow custom sorting and ordering of imports.
See astral-sh/ruff#2419.
WeilerP added a commit to WeilerP/tradeSeq-py that referenced this issue Mar 15, 2023
* Exclude import sorting in `ruff`

As of now, `ruff` does not allow custom sorting and ordering of imports.
See astral-sh/ruff#2419.

* Run pre-commit hooks

Apply `isort` changes.
WeilerP added a commit to adamgayoso/scvelo that referenced this issue Mar 15, 2023
Reintroduce `isort` instead of relying on `ruff`. As of now, `ruff`
does not support custom sections and ordering. See
astral-sh/ruff#2419.
@vviikk
Copy link

vviikk commented Mar 31, 2023

Is anyone working on this? We are now using Ruff in a huge repo ❤️ . But isort remains in use because have a custom section ordering.

Thanks!

@charliermarsh
Copy link
Member

@vviikk - To my knowledge it's not currently being worked on. Up for grabs if someone wants to take it :)

@QuentinN42
Copy link

I will be happy to contribute on this but I didn't have much time last month.
Happy to have ideas of implementations or discussions about the following options :

  • The "copy isort features 1 to 1"
    • simple migration
    • not really scalable
  • The sections as a list with section-order (section define an import block but we keep the section-order in the isort block)
    • Can have custom params in each section
    • Can copy section from others repo easily
    • The import order is easy to view / edit
    • The import order needs to be edited each time a new section is added
  • The sections as a list with level (section define an import block with a level and the section ordering is computed at runtime)
    • Can have custom params in each section
    • Can copy section from others repo easily
    • The import order may be hard to view / edit in repo with a lot of sections (but can be mitigated with levels with large padding)
    • The import order is automatically computed each time a new section is added

@rpep
Copy link

rpep commented Apr 4, 2023

I wish I had some way to assess the importance or popularity of this functionality, since it does introduce a bunch of complexity. Does anyone want to make a strong case for it

Just wanted to add a +1 for this - at the moment this would be something that blocks us from adopting Ruff wholesale on a large commercial project. For us, we want (as we do this with isort already) separation out of core Django and Django REST Framework imports from OS imports and then some other tools we use (Pandas, NumPy).

@charliermarsh
Copy link
Member

charliermarsh commented Apr 13, 2023

Implemented in #3900.

The API looks like this:

[tool.ruff.isort]
section-order = ["future", "standard-library", "third-party", "django", "first-party", "local-folder"]

[tool.ruff.isort.sections]
django = ["django"]

@mastacheata
Copy link

@charliermarsh I think your example for how the API looks is missing the django section in the section-order part (unless this changed between creating the PR and accepting it)

@charliermarsh
Copy link
Member

@mastacheata - Hah, thank you, fixed 🤦

@rpep
Copy link

rpep commented Apr 17, 2023

Looks great! Thank you for implementing this so quickly :)

@charliermarsh
Copy link
Member

I think someone commented-then-deleted but yes, this'll go out in the next release this week :)

@huynguyengl99
Copy link

Hi, the comment was mine, but after checking that the main code is not the pypi latest distribution, I have deleted it 😂 But anyway, thank you a lot for the awesome tool, I will completely migrate to Ruff after the Isort custom feature release 😁.

paddyroddy added a commit to epitools/epitools that referenced this issue Apr 21, 2023
Avasam added a commit to Toufool/AutoSplit that referenced this issue Apr 21, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
paddyroddy added a commit to paddyroddy/python-template that referenced this issue May 3, 2023
@ddahan
Copy link

ddahan commented May 4, 2023

Thanks for this, it's amazing to be able to get rid of another package!

Is there any plan to update the related JSON schema? I wrote an issue about it: #4218

@JonathanPlasse
Copy link
Contributor

Avasam added a commit to Toufool/AutoSplit that referenced this issue May 23, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
Avasam added a commit to Toufool/AutoSplit that referenced this issue May 23, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
isort Related to import sorting
Projects
None yet
Development

No branches or pull requests