Replace bandit, isort, pylint, pyupgrade, and flake8 plus plugins with ruff

[cclauss]

Ruff supports over 500 lint rules including bandit, isort, pylint, pyupgrade, and flake8 plus its plugins and is written in Rust for speed.

The ruff Action uses minimal steps to run in ~10 seconds, rapidly providing intuitive GitHub Annotations to contributors.

The Required in the checks below should be:

1. Removed from flake8-annotation and added to ruff-annotation which replaces it.
2. Removed from isort and added to ruff which replaces it.

[larsoner]

This appears to be working, but in order to check:

Replace bandit, isort, pylint, pyupgrade, and flake8

Could you intentionally create an error with each one of these? That way we can see that it works. Then you can git revert that commit hash, push, and we can merge.

[cclauss]

Could you intentionally create an error with each one of these?

I did so in my creation of this pull request. It was trial and error all the way to this exact configuration.

You are not alone in thinking that anything so complex that happens in subsecond time must be broken. That sentiment is mentioned multiple times in https://beta.ruff.rs/docs/#testimonials

I would encourage you to pipx install ruff and then check out this branch and modify the ruff with settings in pyproject.toml and do ruff . to see how it all works.

[DimitriPapadopoulos]

Just a thought about the ruff pitch itself:

[...] written in Rust for speed.

I would argue that nowadays “efficiency” in terms of carbon footprint is more important than speed in CI code. That said, I am certain that faster code is more efficient – unless speed is achieved only by parallelization.

My only concern was that ruff might be hard to install for Python developers, but it can actually be installed with a simple pip install ruff.

[larsoner]

You are not alone in thinking that anything so complex... I would encourage you to pipx install ruff and then check out this branch and modify the ruff with settings in pyproject.toml and do ruff . to see how it all works.

This isn't quite what I'm getting at -- I have used ruff in other projects and I like it quite a lot. I just want to see on GitHub (now) that the configuration you've added to this project/CIs/whatever does what it's supposed to do when people do bad things in their PRs. So I'm asking for you to add a tiny commit here that does some bad things style-wise so we can confirm by eye that how you've configured ruff here will work like our existing setup. Then we revert the commit. Without doing this, then some of these replaced checks might have been incorrectly replaced and we wouldn't know for perhaps a long time.

Make sense?

[larsoner]

Like this -- will it actually annotate a flake failure the way actions-flake would have? Let's find out by testing it here!

[cclauss]

Suggested change

	- name: Ruff with annotations
	run: ruff --format=github --select=ANN --ignore=ANN101,ANN401 .
	- name: Ruff with annotations
	run: ruff --select=ANN --ignore=ANN101,ANN401 .

Good call. As I say below, this private run confuses me. This can generate errors about Python annotations but those errors do not become GitHub annotations.

[larsoner]

Do we need this at all given that it's run in .github/workflows/codespell-private.yml already?

[cclauss]

I do not really understand the purpose / differentiation of "private" but it is only testing --select=ANN which is a much narrower set of rules than this Ruff action.

I disabled ANN101 in one and ANN401 in the other so we could see that both do their thing.

[cclauss]

Errors galore in Let's create errors in bandit, isort, pylint, pyupgrade, and flake8

[cclauss]

@larsoner Please review the error annotation and let me know when I can revert the intentional errors.

[larsoner]

Yes the annotations look great, feel free to revert!

[cclauss]

Reminder:
The Required in the checks below should be:

• Removed from flake8-annotation and added to ruff-annotation which replaces it.
  Removed from isort and added to ruff which replaces it.

[larsoner]

Thanks @cclauss !

[cclauss]

Thanks for codespell... It is awesome!

[DimitriPapadopoulos]

Unfortunately, the ruff action complains about issues that are not relevant to a pull request:

Error: codespell_lib/_codespell.py:993:5: C901 `main` is too complex (50)
Error: codespell_lib/_codespell.py:993:5: PLR0912 Too many branches (53/48)
Error: codespell_lib/_codespell.py:993:5: PLR0915 Too many statements (114/111)

See for example #2767. What's the best course of action here? Get rid of C901, PLR0912 and PLR0915? Tell ruff to disregard anything that is not relevant to the current patch? But how?

[cclauss]

The best solution would be to improve the code by reducing its complexity. These are well-proven indicators of code that will be difficult to maintain.

If that is not an option then modify the following lines in pyproject.toml

• max-complexity = 45 # Ideally this should be 10... http://en.wikipedia.org/wiki/Cyclomatic_complexity
• max-branches = 48
• max-statements = 111

[DimitriPapadopoulos]

Modifying the code is a good long-term idea.

The problem here is that these rules break even small merge requests that do not add complexity, but only slightly modify existing complex code.

[cclauss]

We put guardrails in place so that cars do not drive off the cliff. When maintainers review pull requests, they should encourage contributors to take an active role in keeping the garden well-maintained.

[DimitriPapadopoulos]

You cannot expect contributors fixing a bug by modifying a few lines of code to refactor the whole existing code base instead.

Have you had a look at #2767? I cannot refactor the whole codespell codebase, it would take months or years given the resources I can put in it.

Reducing code complexity is a good long-term idea. But you cannot stop any short-term maintenance effort until the whole code base has been refactored.

[larsoner]

@cclauss I agree we should try to make our code better when possible. But @DimitriPapadopoulos if you think the code is clean enough and ruff is being too picky in #2767 I think we can relax the thresholds (a little bit) for now. When someone is motivated to clean up the code base I'm sure there are lots of potential targets!

[DimitriPapadopoulos]

It's just that the codespell code is already too complex. So, small change might raise CI alerts and put off new contributors. Complexity metrics are OK, of course, however I suspect some degree of flexibility is useful too. Not sure how to best handle flexibility.

[larsoner]

Ultimately things like code coverage and complexity checkers can act as a guide not an absolute rule. So on a case-by-case basis we can relax/ignore some checks when it makes sense

[cclauss]

Agreed that these are not absolute rules and flexibility is required. My sense is that the best way to provide that flexibility is via a discussion during pull request review like #2767 (review) . I have found that contributors usually speak up when confused or blocked.