Add pre-commit

[cbrnr]

This should include all style and other code quality checks. Currently, I've added Black, Ruff, and Codespell. I'll start with their default configs, but will add custom settings (probably in pyproject.toml).

To do:

• Add custom config
• Remove "codespell and flake" workflow as required
• Make Pre-commit workflow required before running other workflows
• Wait for MAINT: Use black #10641 before enabling Black
• Add other hooks that are not available in Codespell, Ruff, and Black

[cbrnr]

It seems like the tomli dependency lines can be removed with the next release of Codespell (codespell-project/codespell#2644).

[larsoner]

New codespell released https://pypi.org/project/codespell/2.2.3/

[cbrnr]

That was quick, thanks for the heads up! I will remove the tomli lines.

[larsoner]

... tomli is still needed for <3.11, though, assuming you want toml support

[cbrnr]

Yes, but I'm running pre-commit on Python 3.11.

[cbrnr]

Now I wonder why Codespell found these errors in the Azure job but neither in the old GitHub Action nor the new Pre-Commit one:

mne/decoding/tests/test_csp.py:263: shuold ==> should
mne/forward/forward.py:1264: matirx ==> matrix
mne/utils/check.py:850: instatiating ==> instantiating
doc/changes/0.24.inc:296: identitiy ==> identity

OK, the last one is clear, because I didn't add .inc files, but the other three?

Edit: Now the other jobs also found these misspellings, but why not previously?

[drammock]

Now I wonder why Codespell found these errors in the Azure job but neither in the old GitHub Action nor the new Pre-Commit one:

my suspicion would be that the azure job is somehow getting different dictionary settings or using a newer codespell version.

[drammock]

crossref to #11547

[cbrnr]

OK, I fixed it here as well.

[cbrnr]

But this begs the question: why do we run Codespell twice?

[drammock]

But this begs the question: why do we run Codespell twice?

no idea.

[cbrnr]

I'll remove it then.

[cbrnr]

For flake8 we currently have:

exclude = __init__.py,constants.py,fixes.py,resources.py,*doc/auto_*,*doc/_build*,build/*

And for pydocstyle:

match_dir = ^(?!\.|doc|tutorials|examples|logo|icons).*$
match = (?!tests/__init__\.py|fixes).*\.py

[larsoner]

@cbrnr I'd like to have this sooner rather than later because the time it takes to make flake locally is starting to bug me :)

Want me to push commits to finish or can you do it in the next couple of days?

[cbrnr]

Thanks @larsoner, please feel free to finish this up. I still don't know how to run different configurations for ruff-specific tools (i.e. pydocstyle should only be run in mne/, but everything else in all directories). I also don't know how you would like to handle pydocstyle-related warnings – I'd prefer to fix them, but most warnings seem to be outside of mne/ anyway.

I'm happy to review the final PR of course!

[larsoner]

Okay @cbrnr I think this is working, feel free to take a look. I think we can wait on the other points in the top comment. For the GH Actions job dependencies we should first in a separate PR put all our pytest actions in one YAML (which we only had separate for back when you couldn't restart just failed runs) along with this new precommit one. Then we can make the pytest ones depend on the style job very easily.

[larsoner]

@drammock could you also look and make sure you're okay with these changes? Basically:

1. Replace flake8 and pydocstyle with ruff which does the same work much faster
2. Add .pre-commit-config where people can do pre-commit install --install-hooks and have git commit reject when style is not satisfied. Or they don't have to do this and the CIs will yell at them, as usual.
3. Alias make flake and make pep to use the pre-commit config
4. Update contrib docs to mention pre-commit

[larsoner]

I think we can ignore the SphinxWindows failures... they don't seem related.

[drammock]

GitHub is highlighting my suggested changes in red, but AFAIK it is valid TOML to include line comments in this way. Is pyproject.toml more narrowly restricted?

[larsoner]

Locally your changes seemed to work fine so I've committed all of them, and also removed D107. It was probably made redundant at some point and we never noticed. Will merge once everything except SphinxWindows comes back green, thanks @cbrnr !

[cbrnr]

Thank you!