Closed boxydog closed 2 months ago
By the way, even the existing pre-commit filters fail (see below). I'd probably fix that first.
Then I'd put pre-commit run --all
into the github workflow. Then I'd start adding new filters. Probably separate PRs?
=====
$ pre-commit run --all
check for added large files..............................................Passed
check python ast.........................................................Passed
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
detect private key.......................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook
Fixing pelican/tests/content/empty_with_bom.md
Fixing pelican/themes/simple/templates/category.html
Fixing pelican/tests/content/article_with_uppercase_metadata.rst
Fixing pelican/tests/TestPages/draft_page_markdown.md
Fixing pelican/tests/content/bloggerexport.xml
Fixing pelican/themes/notmyidea/static/css/reset.css
Fixing pelican/tests/content/2012-11-30_md_w_filename_meta#foo-bar.md
Fixing samples/content/pages/hidden_page.rst
Fixing pelican/themes/simple/templates/author.html
Fixing pelican/tests/content/wordpress_content_encoded
Fixing pelican/tests/content/article_without_category.rst
Fixing MANIFEST.in
Fixing .coveragerc
Fixing pelican/tests/content/article_with_markdown_markup_extensions.md
Fixing docs/_static/pelican-logo.svg
forbid new submodules................................(no files to check)Skipped
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook
Fixing docs/_static/theme_overrides.css
Fixing samples/content/unbelievable.rst
Fixing pelican/tests/content/wordpressexport.xml
isort is already listed among the dev dependencies in pyproject.toml
but hasn't made its way to the pre-commit
configuration because the existing codebase does not yet have Black, Flake8, and isort applied en-masse. That is intentional, because we didn't want to cause all the existing PRs in the queue to instantly become unmergeable without fixing a bunch of conflicts caused by mass-applying code style changes in master
.
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 pyproject.toml
and the Pre-commit configuration.
There is already an open issue regarding using Ruff instead of a half-dozen different tools: #3123
That is intentional, because we didn't want to cause all the existing PRs in the queue to instantly become unmergeable without fixing a bunch of conflicts caused by mass-applying code style changes in
master
.
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.
That is intentional, because we didn't want to cause all the existing PRs in the queue to instantly become unmergeable without fixing a bunch of conflicts caused by mass-applying code style changes in
master
.I should point out that #2898 was never merged for the same reason noted above.
Ha ha, I see this comes up over and over.
I still think ruff is unlikely to replace everything above. For example, pyupgrade.
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 invoke lint
checks.
Ha ha, I stand corrected!
From https://docs.astral.sh/ruff/:
Ruff can be used to replace Flake8 (plus dozens of plugins), Black, isort, pydocstyle, pyupgrade, autoflake, and more, all while executing tens or hundreds of times faster than any individual tool.
In that case, I would look through my list and see if there's anything I like that's not in 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 https://github.com/astral-sh/ruff/issues/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.
select = ["E4", "E7", "E9", "F"]
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.
[tool.ruff.lint]
# default rules include pyflakes ("F")
# see https://docs.astral.sh/ruff/configuration/#using-pyprojecttoml
# "F" contains autoflake, see https://github.com/astral-sh/ruff/issues/1647
# add more rules
extend-select = [
# TODO: "A", # flake8-builtins
# TODO: "ARG", # flake8-unused-arguments
"B", # flake8-bugbear
"BLE", # flake8-blind-except
# TODO: Do I want "COM", # flake8-commas
"C4", # flake8-comprehensions
# TODO: "DJ", # flake8-django
# TODO: "DTZ", # flake8-datetimez
# TODO: "EM", # flake8-errmsg
"EXE", # flake8-executable
# TODO: "FURB", # refurb
# TODO: "FBT", # flake8-boolean-trap
# TODO: "G", # flake8-logging-format
"I", # isort
"ICN", # flake8-import-conventions
"INP", # flake8-no-pep420
# TODO: "INT", # flake8-gettext
"ISC", # flake8-implicit-str-concat
# TODO: "LOG", # flake8-logging
"PERF", # perflint
"PIE", # flake8-pie
"PL", # pylint
"PYI", # flake8-pyi
# TODO: "RET", # flake8-return
"RSE", # flake8-raise
"RUF",
# TODO: "SIM", # flake8-simplify
"SLF", # flake8-self
"SLOT", # flake8-slots
"TID", # flake8-tidy-imports
"UP", # pyupgrade
"Q", # flake8-quotes
"TCH", # flake8-type-checking
"T10", # flake8-debugger
"T20", # flake8-print
# TODO: "S", # flake8-bandit
"YTT", # flake8-2020
# TODO: add more flake8 rules
]
ignore = [
"RUF012"
]
[tool.ruff.lint.extend-per-file-ignores]
"**/migrations/*" = ["Q"]
"**/management/commands/*" = ["T20"]
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.
Maybe this can be closed?
There is more work to stop ignoring a bunch of the ruff warnings. Not sure if that should be tracked here or elsewhere.
Let's do follow-up Ruff-related work in subsequent pull requests. Thanks, @boxydog!
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.