Overhaul linting and formatting

niksirbi commented 3 months ago

Description

What is this PR

[ ] Bug fix
[x] Addition of a new feature
[ ] Other

Why is this PR needed?

We use ruff for linting and black for auto-formatting. With the introduction of ruff.format (a drop-in replacement for black, but faster) there is no longer need to be using black. This prompted me to overhaul our entire lint/format setup.

What does this PR do?

I went through the scientific python development guide for style and static checks and applied the recommendations that make sense for movement.

In summary these are:

switch from black to ruff.format
add some more ruff rules. I ended up with the following set (which includes the most popular ruff rules):
```
lint.select = [
  "E",   # pycodestyle errors
  "F",   # Pyflakes
  "UP",  # pyupgrade
  "I",   # isort
  "B",   # flake8 bugbear
  "SIM", # flake8 simplify
  "C90", # McCabe complexity
]
```
Of those, we were already using E, F, and I. With the addition of UP, B, SIM, and C90 I discovered and fixed a few minor problems that the other checks had missed. That said, there were only a few extra issues, which means we are already great Python coders 😉 .
- add some more recommended pre-commit hooks we were missing, such as check-added-large-files and check-yaml.
- add pre-commit hooks to specifically catch some easy-to-make rst mistakes (e.g. single-vs-double backticks for monospace formatting)

What does this PR NOT do? I also considered adding the "D" ruff rule (pydocstyle) to enforce consistent formatting for our docstrings (according to PEP8 recommendations). I think this is super useful, but currently requires some manual work to update lots of our non-compliant docstrings (135 non auto-fixable violations!). I will open an issue to do this later.

References

If we end up being happy with the new setup, we may consider porting it over to the python-cookiecutter.

Does this PR require an update to the documentation?

The contributed guidelines have been updated accordingly.

Checklist:

[x] The code has been tested locally
[ ] Tests have been added to cover all new functionality
[x] The documentation has been updated to reflect any changes
[x] The code has been formatted with pre-commit

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.63%. Comparing base (5123a41) to head (f3b433c).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #156 +/- ## ========================================== + Coverage 99.28% 99.63% +0.35% ========================================== Files 9 9 Lines 557 554 -3 ========================================== - Hits 553 552 -1 + Misses 4 2 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

niksirbi commented 3 months ago

Tagging @neuroinformatics-unit/behaviour and @b-peri, because these changes will affect workflows for all.

lochhh commented 2 months ago

Thanks again @niksirbi ! Happy to merge once the following two things are addressed:

[x] The mixed-line-ending check fails with 52 files when running pre-commit run --all-files locally on Windows. Could you try this on a Mac?
[x] Add the "D" ruff rule (pydocstyle) but commented out in prep for the PR to fix the violations.

adamltyson commented 2 months ago

Assuming these changes aren't too specific to movement, could they be added to the cookiecutter, and gradually rolled out to other packages? I'm very keen that our tools don't start to diverge in terms of tooling.

niksirbi commented 2 months ago

Assuming these changes aren't too specific to movement, could they be added to the cookiecutter, and gradually rolled out to other packages? I'm very keen that our tools don't start to diverge in terms of tooling.

Yes, the plan was to roll them out in movement first, for testing, and after a few weeks (assuming all is well) roll them out to the cookiecutter + other packages.

niksirbi commented 2 months ago

The mixed-line-ending check fails with 52 files when running pre-commit run --all-files locally on Windows. Could you try this on a Mac?

@lochhh I can't reproduce this on Mac. Running pre-commit run --all-files on my M2 Mac finds no issues. Could you check whether your IDE is configured to automatically use CRLF for line endings (i.e. \r\n, that's the Windows way)? On Unix systems it's LF (\n) and that's what the pre-commit hook enforces (and in theory, auto-fixes).

lochhh commented 2 months ago

The mixed-line-ending check fails with 52 files when running pre-commit run --all-files locally on Windows. Could you try this on a Mac?

@lochhh I can't reproduce this on Mac. Running pre-commit run --all-files on my M2 Mac finds no issues. Could you check whether your IDE is configured to automatically use CRLF for line endings (i.e. \r\n, that's the Windows way)? On Unix systems it's LF (\n) and that's what the pre-commit hook enforces (and in theory, auto-fixes).

Thanks for looking into this. Can confirm it is indeed my IDE that's automatically converting the line endings.