-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
More pre-commit filters, e.g. isort #3216
Comments
By the way, even the existing pre-commit filters fail (see below). I'd probably fix that first. Then I'd put =====
|
isort is already listed among the dev dependencies in Part of my vision for the upcoming sprint was to merge or close most of the existing pull requests, and then apply code style standards to the entire codebase, along with adding more code style configuration to There is already an open issue regarding using Ruff instead of a half-dozen different tools: #3123 |
I should point out that #2898 was never merged for the same reason noted above. |
I see. Well, feel free to close this then. I still think ruff is unlikely to replace everything above. For example, pyupgrade. So, maybe ruff plus others? In general I think more pre-commit, more better. |
Ha ha, I see this comes up over and over. |
Ahem: https://docs.astral.sh/ruff/rules/#pyupgrade-up But your point stands, in the sense that Ruff probably won't do everything. To which I would say replacing everything isn't the intent. The intent is to reduce overheard by limiting the number of tools that have to be installed and configured. So yeah — Ruff plus others. And if you look at how I've done things in some of the latest plugins, you'll see that Pre-commit should be completely in sync and in unison with what |
Ha ha, I stand corrected! From https://docs.astral.sh/ruff/:
In that case, I would look through my list and see if there's anything I like that's not in ruff! |
And I see nothing big missing! Well done Ruff. There might be some of those "pre-commit" checks (e.g., no-commit-to-branch), but that's littler stuff. |
Ruff still seems good, but FYI there is a bunch of pylint it does not implement. See astral-sh/ruff#970. That is awkward, because pylint is pretty slow, so you might not want to use it. You likely already know this. |
Yeah, I think I would rather start with a smaller set of rules and then expand the “strictness” as time goes on. Even with the smaller set of rules that Ruff supports, I have put myself in situations where I enabled some rules and then spent unholy amounts of time trying to manually cajole an existing codebase to pass that relatively small set of rules. So unless there’s a strong reason not to do so, I am totally okay with skipping Pylint for now. |
I would be willing to grind away at the menial task of getting some rules to pass if you wish. I find it satisfying, not saying it's needed. Let me know. |
On the contrary, that would be really helpful. As I mentioned, my only hesitation at the moment is causing a bunch of merge conflicts with existing PRs in the queue. Would you consider helping out with existing PR triage? The sooner we get them either merged or closed, the sooner we can start applying code style without fear of causing widespread merge conflicts. |
@boxydog: I think the pre-commit configuration is now fairly robust. If there's anything specific that you think is missing, by all means send a pull request to address it. Thanks for helping out with this! |
I can't re-open this (no button), but you should consider the following list:
pre-commit:
Lucas-C pre-commit:
If you wish, I can open another issue about it. |
FYI, here is the Ruff default config (the one we're currently using): https://docs.astral.sh/ruff/configuration/#using-pyprojecttoml.
There is tons of Ruff (the majority) that is not enabled. |
Below is an abbreviated version of the Ruff config in the pyproject.toml of one of my projects. A bunch of the rules are commented-out as a signal to me that I tried it, it complained, and I was not willing to fix it at the moment.
|
I re-opened the issue. If you think that some of the aforementioned additional rules would be beneficial to the project, feel free to submit a pull request that adds them.
|
Okay, submitted 3239. |
Feature Request
I submitted a pull request that passed pre-commit but failed lint checks, because of import order.
isort is a pre-commit filter that I think would've fixed my import order. I feel like we should add it.
Some others I'd recommend, in rough order:
pyupgrade
autoflake
black
pylint
pre-commit check-case-conflict, check-docstring-first, check-merge-conflict, check-symlinks, debug-statement, mixed-line-ending, forbid-crlf, forbid-tabs
I understand that adding each of these would result in code shift getting the project to pass. My approach would be to add them all, see which are easy to keep, and drop the rest for now. Time box to a couple hours.
It has this problem that it would conflict with every other PR, so I would want permission to get it in fairly quickly, otherwise resolving conflicts becomes too painful.
Below is a sample pre-commit from one of my projects. It's not a perfect fit, but I could adjust it. For example, versions. I was targeting newer pythons, but could tune it back to python 3.7, which seems to be the oldest one you test.
The text was updated successfully, but these errors were encountered: