[INFRA] Initial formatting with black and precommit #3484

Remi-Gau · 2023-02-03T16:13:42Z

Relates to #2528

Changes proposed in this pull request:

Set up pre-commit hooks to work only on the nilearn/decomposition module
- Black
- Flake8
- Codespell
- Several "classical" hooks from pre-commit (json, yml parsing, merge conflicts...)
- pyupgrade to update python syntax to use versions of the language >= 3.6
Apply the hooks on nilearn/decomposition using pre-commit run -a
Adapt Flake8 config in setup.cfg to play nice with black
Add a pyproject.toml to host the config for black.
Add a flake8 CI workflow to run on entire python files, only for this module
Remove flake8 workflow that worked just on file diff
update contributing to mention the install and usage of black and pre-commit

github-actions · 2023-02-03T16:13:56Z

👋 @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.

PR has an interpretable title.
PR links to Github issue with mention "Closes #XXXX"
Code is PEP8-compliant.
Changelog or what's new entry in "doc/changes/latest.rst"
(Bug fixes) There is at least one test that would fail under the original bug conditions.
(New features) There is at least one unit test per new function / class.
(New features) The new feature is demoed in at least one relevant example.

We will review it as quick as possible, feel free to ping us with questions if needed.

Remi-Gau · 2023-02-03T16:21:01Z

setup.cfg

@@ -72,6 +72,7 @@ dev =
 # W503: line break before binary operator
 # W504: line break after binary operator
 ignore=E402, W503, W504, W605
+max-line-length = 91


I could bring this number down but if I do Flake8 would complain for some files in this PR.

Other possibility I could try is setting the config for black for shorter lines.

Well, I'm just curious about where these numbers come from... Ten we simply need to decide collectively what we think is best for readability.
Actually, I'm a bit surprised to see how these things fluctuate across years ;-)

For the record there is a quite bit of variability indeed.

Did set things at 90 for now (because 91 is just a weird number).

flake8_line_length.txt

I think the flake8 default is 79 and that's what we've been using before our PEP8 CI broke

Don't feel strongly about any specific value: will change the value and adapt.

Remi-Gau · 2023-02-03T16:22:21Z

.pre-commit-config.yaml

+# See https://pre-commit.com for more information
+# See https://pre-commit.com/hooks.html for more hooks
+
+files: "nilearn/decomposition"


Adding new modules and other files will have to be done by modifying this line.
It should be a valid python regex.

.github/workflows/pep8.yml

.pre-commit-config.yaml

Remi-Gau · 2023-02-03T16:27:01Z

While the code formatting is transitioning it may be good to:

mention it in the contributing.md
also add a mention in there about pre-commit

Remi-Gau · 2023-02-03T16:31:07Z

Regarding mentions in the changelog, it may make sense to have a single entry once most of the code has been blackified (blacked?) rather than one per PR, no?

Remi-Gau · 2023-02-03T16:35:16Z

I could not see any code in the reformated files that contained code that had a "table like" layout that would have been mangled by black. Let me know, if you think I overlooked something.

codecov · 2023-02-03T16:57:57Z

Codecov Report

Merging #3484 (0cb1d65) into main (64d51af) will increase coverage by 0.04%.
The diff coverage is 86.88%.

@@            Coverage Diff             @@
##             main    #3484      +/-   ##
==========================================
+ Coverage   90.94%   90.99%   +0.04%     
==========================================
  Files         133      133              
  Lines       15335    15343       +8     
  Branches     3024     3028       +4     
==========================================
+ Hits        13947    13961      +14     
+ Misses        820      817       -3     
+ Partials      568      565       -3

Impacted Files	Coverage Δ
nilearn/decomposition/dict_learning.py	`89.06% <72.72%> (ø)`
nilearn/decomposition/canica.py	`91.66% <80.00%> (ø)`
nilearn/decomposition/_base.py	`94.87% <91.89%> (+1.79%)`	⬆️
nilearn/decomposition/__init__.py	`100.00% <100.00%> (ø)`
nilearn/decomposition/_multi_pca.py	`100.00% <100.00%> (ø)`
nilearn/image/image.py	`95.73% <0.00%> (ø)`
nilearn/maskers/base_masker.py	`83.95% <0.00%> (ø)`
nilearn/maskers/multi_nifti_maps_masker.py	`95.65% <0.00%> (ø)`
nilearn/maskers/multi_nifti_labels_masker.py	`95.65% <0.00%> (ø)`
nilearn/_utils/docs.py	`92.12% <0.00%> (+0.06%)`	⬆️
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

NicolasGensollen

Great ! Thanks for taking care of that @Remi-Gau ! 👍
I went through the changes and they all LGTM.
Concerning the documentation, I also think we should add a few things related to setting up the pre-commit hook, probably here: https://nilearn.github.io/stable/development.html#setting-up-your-environment
Also, we should make sure our coding style guidelines (https://nilearn.github.io/stable/development.html#coding-style) are consistent with the linting we're enforcing.
Concerning the changelog, I also agree it might be cleaner to have only a single entry for the formatting of the whole code base rather than one entry per PR.
That being said, if we keep the one PR per module style, and different contributors end up doing them, I don't think that it would be a huge deal either...

.pre-commit-config.yaml

Remi-Gau · 2023-02-03T18:41:45Z

Ok I will update the contributing (but not today).

Also I am realizing that unless we use something like pre-commit ci, it may be good to have something in CI that runs black to at least say which files in the PR should be formatted with black.
Otherwise someone who touches those files later and use pre-commit or black will see some line changes that he or she has not worked on.

bthirion

The changes so far LGTM.

…one is disabled

bthirion

LGTM otherwise.

Remi-Gau · 2023-02-06T08:52:39Z

OK I think I am done with this PR.

Let me know if I am missing anything or if I should change anything.

ymzayek

Thanks, this set up looks great!

.github/workflows/pep8.yml

CONTRIBUTING.rst

ymzayek · 2023-02-06T10:49:38Z

Can you also remove the recommendation in step 3 under here: https://nilearn.github.io/stable/development.html#id3 and replace with a code block of syntax to flake8 a file?

Remi-Gau · 2023-02-06T13:05:14Z

Can you also remove the recommendation in step 3 under here: https://nilearn.github.io/stable/development.html#id3 and replace with a code block of syntax to flake8 a file?

done

also added mention of black and pre-commit

ymzayek · 2023-02-06T14:03:07Z

The documentation builder failure is unrelated, see actions/runner-images#7048

For the codecov patch failure, I'm guessing we're gonna see this when reformatting the code base as it will pick up on untested lines that are changed by the reformatting since it runs on the diff. I think we can ignore for these PRs but it could be an idea to start increasing the testing coverage in general for nilearn (in separate PRs based on these reports).

ymzayek

Thanks @Remi-Gau!

NicolasGensollen

LGTM, thanks @Remi-Gau !

bthirion

LGTM, thx.

Remi-Gau · 2023-02-07T06:15:54Z

.pre-commit-config.yaml

+      - id: trailing-whitespace
+
+  - repo: https://github.com/psf/black
+    rev: 22.10.0


Black had just released a new version, so I should probably bump this before merging.

Sure, you can go ahead.

Done.

Once this is merged I will open a separate PR with a workflow to check and update the hooks version weekly so we don't have to remember to regularly update this.

Great thanks!

ymzayek · 2023-02-07T14:48:22Z

Ok merging

Remi-Gau added 6 commits February 3, 2023 16:42

add pre-commit config

a43a912

upate pre-commit config

70ac784

update setup.cfg to pacify pre-commit without touching any files

97bab37

apply black to nilearn/decomposition

fb86a24

update precommit config

0129b45

update workflow to run on entire files

1d6d371

Remi-Gau changed the title ~~[INFRA] Formatting with black and precommit~~ [INFRA] Initial formatting with black and precommit Feb 3, 2023

Remi-Gau commented Feb 3, 2023

View reviewed changes

.github/workflows/pep8.yml Outdated Show resolved Hide resolved

Remi-Gau commented Feb 3, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

Remi-Gau marked this pull request as ready for review February 3, 2023 16:31

NicolasGensollen reviewed Feb 3, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

bthirion reviewed Feb 3, 2023

View reviewed changes

Remi-Gau added 10 commits February 4, 2023 09:02

create separate workflow to run flake8 on whole file as the original …

d976714

…one is disabled

add black config and adapt flake8 config

d1bc1b5

adapt line length

bd50901

update hooks version and remove checking large files

82060b2

add black config in pre-commit

d91f87c

add warning in contributing about fading to black

34783d4

update contributing

4333420

add workflow to run black on diff

440a128

change job name

0a1ffda

actually install black

139854f

Remi-Gau added 4 commits February 4, 2023 17:28

add path to black command

9dfd3b8

do not use config

90e6726

run black on entire files

d3e726f

rm comment

d8c354b

Remi-Gau mentioned this pull request Feb 4, 2023

Automate the Formatting stuff #2528

Closed

bthirion reviewed Feb 5, 2023

View reviewed changes

Remi-Gau added 3 commits February 6, 2023 09:30

integrate some of sourcery suggestions

7b330c3

apply pyugrade

647efda

set line length to 90

5b8d0f9

Remi-Gau added 2 commits February 6, 2023 10:08

apply more sourcery refactoring

f879b0d

set line length to 79

d81ab91

ymzayek reviewed Feb 6, 2023

View reviewed changes

.github/workflows/pep8.yml Outdated Show resolved Hide resolved

CONTRIBUTING.rst Outdated Show resolved Hide resolved

This was referenced Feb 6, 2023

[FIX] Flake8 latest release raises error and includes backwards incompatilble change #3420

Closed

All CI failing because flake8 --diff option has been removed #3431

Closed

rm flake8 diff workflow

c17acb7

update CONTRIBUTING

f99a0c8

ymzayek approved these changes Feb 6, 2023

View reviewed changes

NicolasGensollen approved these changes Feb 6, 2023

View reviewed changes

bthirion approved these changes Feb 6, 2023

View reviewed changes

Remi-Gau commented Feb 7, 2023

View reviewed changes

bump black version in pre-commit

0cb1d65

ymzayek merged commit 77c1d78 into nilearn:main Feb 7, 2023

Remi-Gau mentioned this pull request Feb 7, 2023

[INFRA] add a GH workflow to regularly update pre-commit hooks #3490

Closed

Remi-Gau mentioned this pull request Mar 14, 2023

[MAINT] Minor refactor in decomposition, decoding, image modules #3590

Merged

Remi-Gau mentioned this pull request Apr 6, 2023

[MAINT] add git blame ignore #3663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INFRA] Initial formatting with black and precommit #3484

[INFRA] Initial formatting with black and precommit #3484

Remi-Gau commented Feb 3, 2023 •

edited

github-actions bot commented Feb 3, 2023 •

edited by ymzayek

Remi-Gau Feb 3, 2023

bthirion Feb 5, 2023

Remi-Gau Feb 6, 2023

ymzayek Feb 6, 2023

Remi-Gau Feb 6, 2023

Remi-Gau Feb 3, 2023

Remi-Gau commented Feb 3, 2023

Remi-Gau commented Feb 3, 2023

Remi-Gau commented Feb 3, 2023

codecov bot commented Feb 3, 2023 •

edited

NicolasGensollen left a comment

Remi-Gau commented Feb 3, 2023

bthirion left a comment

bthirion left a comment

Remi-Gau commented Feb 6, 2023

ymzayek left a comment

ymzayek commented Feb 6, 2023

Remi-Gau commented Feb 6, 2023

ymzayek commented Feb 6, 2023

ymzayek left a comment

NicolasGensollen left a comment

bthirion left a comment

Remi-Gau Feb 7, 2023

ymzayek Feb 7, 2023

Remi-Gau Feb 7, 2023

ymzayek Feb 7, 2023

ymzayek commented Feb 7, 2023

[INFRA] Initial formatting with black and precommit #3484

[INFRA] Initial formatting with black and precommit #3484

Conversation

Remi-Gau commented Feb 3, 2023 • edited

github-actions bot commented Feb 3, 2023 • edited by ymzayek

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Feb 3, 2023

Remi-Gau commented Feb 3, 2023

Remi-Gau commented Feb 3, 2023

codecov bot commented Feb 3, 2023 • edited

Codecov Report

NicolasGensollen left a comment

Choose a reason for hiding this comment

Remi-Gau commented Feb 3, 2023

bthirion left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau commented Feb 6, 2023

ymzayek left a comment

Choose a reason for hiding this comment

ymzayek commented Feb 6, 2023

Remi-Gau commented Feb 6, 2023

ymzayek commented Feb 6, 2023

ymzayek left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek commented Feb 7, 2023

Remi-Gau commented Feb 3, 2023 •

edited

github-actions bot commented Feb 3, 2023 •

edited by ymzayek

codecov bot commented Feb 3, 2023 •

edited