Skip to content

Add linting to the CI setup, using ruff#1857

Open
apdavison wants to merge 1 commit into
NeuralEnsemble:masterfrom
apdavison:ruff
Open

Add linting to the CI setup, using ruff#1857
apdavison wants to merge 1 commit into
NeuralEnsemble:masterfrom
apdavison:ruff

Conversation

@apdavison
Copy link
Copy Markdown
Member

To improve overall code quality, I suggest adding a linter to the CI setup. I've gone with ruff, because I've had good experiences with it (in particular, it is fast), but I'm open to consider a different linter. (Note that ruff also does formatting, it's a drop-in replacement for black)

For now the setup is quite forgiving, but we can toughen it up later.

The CI job first tests there are no regressions in code quality, then produces a report on things that could be improved (linter warnings and errors). As we gradually fix these, the rules can be made stricter.

For now the setup is quite forgiving, but we can toughen it up later.

- name: Run ruff (strict subset)
uses: astral-sh/ruff-action@v3
with:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with:
# Strict subset — only rules the repo already passes, so they can block merges:
# F821 - undefined name (real NameError-class bugs, typos, missing imports)
# F403 - `from module import *` used (hides which names are in scope)
# F405 - name may be undefined, or defined from a star import (F821 that F403 made unprovable)
with:
args: "check --select F821,F403,F405 ."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always forget what those rules mean so I like to document on the spot:

https://github.com/catalystneuro/neuroconv/blob/8d8348f2cbd0cdee74f2eec48d92db469c4c5888/pyproject.toml#L434-L442

Feel free to ignore.

Comment thread pyproject.toml
Comment on lines +150 to +151
[tool.ruff.lint]
select = ["E", "F", "I"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[tool.ruff.lint]
select = ["E", "F", "I"]
# See https://docs.astral.sh/ruff/rules/
select = [
"E", # pycodestyle errors — style/whitespace (line length, blank lines); large backlog
"F", # Pyflakes — real bugs (undefined names, unused/star imports, redefinitions)
"I", # isort — import sorting and grouping
]

Copy link
Copy Markdown
Contributor

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good idea to me. First, we check what we are enforcing so no regression and then we can eventually move others.

Could we:

  1. Have pre-commit configuration on the repo following this one so developers who use it can avoid this automatically (note this is not enforced as installing is opt in)
  2. Unify this with the black action? this will reduce the number of workflows and ruff works as a formater anyway.

Plus, it will reduce PR churn.

Both can be done in a separate PR.

@zm711
Copy link
Copy Markdown
Contributor

zm711 commented Jun 4, 2026

Also in favor of this. I think we remove black and unify to ruff for both. At this point Sam said he is fine for whatever as long as he doesn't have to install something. So this workflow is typically fine for him.

Although in discussion it is not clear the full value added since we already to black once a week we aren't gaining too much other than "more correct" PRs?

@JuliaSprenger
Copy link
Copy Markdown
Member

Hi @apdavison! As far as I see ruff will not provide automatic fixes, but will only flag inconsistent code layout. Won't this be as annoying in the long run as during the pep8speaks times? Or are you planning to switch also from black autoformatting to ruff autoformatting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants