-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Comments
Support for |
Isn't |
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.:
it has nothing to do with relative imports. |
Aren't you mixing https://pycqa.github.io/isort/docs/configuration/options.html#known-first-party https://pycqa.github.io/isort/docs/configuration/options.html#known-local-folder
|
I guess |
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.
→ 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". |
Hey, I'll try to implement the |
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 What do you think about changing that to a BTreeMap or an HashMap to allow runtime update of it (custom categorize) ? |
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 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. |
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 ‘’’ |
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 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 :) |
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 :)). |
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)
|
It's particulary usefull in the monorepo / big projects : Being able to create some groups like :
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 |
But yes I'ts not required at all as a priority. |
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. |
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"]
|
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 PS : this is not a real proposition, I just want to put here as most ideas to be able to take the better decision |
As of now, `ruff` does not allow custom sorting and ordering of imports. See astral-sh/ruff#2419.
As of now, `ruff` does not allow custom sorting and ordering of imports. See astral-sh/ruff#2419.
* 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.
Reintroduce `isort` instead of relying on `ruff`. As of now, `ruff` does not support custom sections and ordering. See astral-sh/ruff#2419.
Is anyone working on this? We are now using Ruff in a huge repo ❤️ . But Thanks! |
@vviikk - To my knowledge it's not currently being worked on. Up for grabs if someone wants to take it :) |
I will be happy to contribute on this but I didn't have much time last month.
|
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). |
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"] |
@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) |
@mastacheata - Hah, thank you, fixed 🤦 |
Looks great! Thank you for implementing this so quickly :) |
I think someone commented-then-deleted but yes, this'll go out in the next release this week :) |
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 😁. |
Once astral-sh/ruff#2419 is released
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
Once astral-sh/ruff#2419 is released
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 |
|
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
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
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:Would become the following in
ruff
:The text was updated successfully, but these errors were encountered: