Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul linting and formatting #156

Merged
merged 16 commits into from
Apr 8, 2024
Merged

Overhaul linting and formatting #156

merged 16 commits into from
Apr 8, 2024

Conversation

niksirbi
Copy link
Member

@niksirbi niksirbi commented Mar 26, 2024

Description

What is this PR

  • Bug fix
  • 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:

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

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@niksirbi niksirbi marked this pull request as ready for review March 26, 2024 17:14
@niksirbi niksirbi requested a review from lochhh March 26, 2024 17:14
@niksirbi
Copy link
Member Author

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

pyproject.toml Show resolved Hide resolved
@lochhh
Copy link
Collaborator

lochhh commented Apr 5, 2024

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

  • 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?
  • Add the "D" ruff rule (pydocstyle) but commented out in prep for the PR to fix the violations.

@adamltyson
Copy link
Member

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
Copy link
Member Author

niksirbi commented Apr 8, 2024

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
Copy link
Member Author

niksirbi commented Apr 8, 2024

  • 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
Copy link
Collaborator

lochhh commented Apr 8, 2024

  • 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.

Copy link

sonarqubecloud bot commented Apr 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@niksirbi
Copy link
Member Author

niksirbi commented Apr 8, 2024

Thanks for verifying @lochhh. I'll merge this now.

@niksirbi niksirbi added this pull request to the merge queue Apr 8, 2024
Merged via the queue into main with commit f90a1c9 Apr 8, 2024
27 checks passed
@lochhh lochhh deleted the ruff-format branch April 8, 2024 14:51
@niksirbi niksirbi mentioned this pull request Apr 18, 2024
7 tasks
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