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
[MAINT] run isort and flake8 on the whole nilearn code base in CI #3651
Conversation
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #3651 +/- ##
==========================================
- Coverage 91.60% 91.60% -0.01%
==========================================
Files 139 139
Lines 16594 16593 -1
Branches 3244 3244
==========================================
- Hits 15201 15200 -1
Misses 1390 1390
Partials 3 3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Once that is done, nilearn should be mostly PEP8 compliant and stay that way. |
will address those in a separate PR |
It seems I had slightly misconfigured isort so that it was not always runnning on the whole codebase: started fixing it here which results in a lot of very minor changes in many files. I also need to finish running it on all the examples: will do this in a subsquent PR as this one is big enough as it is. |
Getting some install errors for some ci that are unrelated to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. Thx !
@@ -24,12 +24,12 @@ | |||
import numpy as np | |||
from matplotlib import gridspec as mgs | |||
from nibabel.spatialimages import SpatialImage | |||
from nilearn._utils import _compare_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8 was flagging them as unnecessary because unused so I removed them thinking "for sure this will break something and this will require a 'noqa' to pacify flake8"
so I am a bit puzzled nothing broke
will try to track down when they became unneeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apparently this was added here: https://github.com/nilearn/nilearn/blame/91039a92982b8f442517a8d552d3dae815d6d33f/nilearn/plotting/img_plotting.py
but it is never used in the whole module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx.
pyproject.toml
Outdated
@@ -62,6 +62,7 @@ doc = [ | |||
"flake8-docstrings", | |||
"black", | |||
"furo", | |||
"isort", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like flake8 didn't catch the correct indentation on this 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait... Does flake8 cover toml files as well?
If not I can at least add a pre-commit hook to help "enforce" some formatting of such support / config files (yaml, json, toml).
There are a couple of options available.
https://pre-commit.com/hooks.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turned out this was a double space instead of a tab...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thx!
Thanks for the reviews y'all. |
Relates to #2528
Changes proposed in this pull request:
This PR should be green once the following have been merged:
This will then mean that the whole code base is PEP8 compliant and should hopefully stay that way.