[STY] Use black and isort to manage library code style

[tsalo]

Closes #219. This PR is a combined effort from @jbteves and @tsalo.

Changes proposed in this pull request:

• Add black and isort settings to package configuration files.
• Ensure black and isort-related linting is done in CI.
• Add badges indicating black coding style.
• Describe the new preferred coding style in contributing documentation.
• Describe precommit hooks that can be used to enforce the coding style.
• Run black and isort on the package.

[tsalo]

I'm leaning toward merging #696 before running black and isort in this PR, since it's so close. I'm happy to deal with conflicts in the other PRs, rather than wait on this one until they're merged.

[jbteves]

Most other PRs are recent enough that they could probably just rebased and then have these run on them.

However, a different concern is that there's actually a few corner-cases for running pre-commit hooks that would automatically run the formatting for you. However, if you forget and then make a commit, it's possible that the commit could abort if black or isort checks fail. Does that seem reasonable to you @emdupre ?

[jbteves]

It seems like there's a bit of a rabbit-hole on pre-commit hooks. The crux of the issue is that you can have separate working and staged areas, but you can't run black or isort on the staged area specifically. So you can't actually check that the staged changes are formatted correctly, only that the working area is. So if you staged something incorrectly formatted, it would go missing. I'm not sure there's a great hook solution without using a fairly full-featured package like pre-commit. However, I'd be in favor of trying something like that. What do people think? Is this worth opening a new issue?

[codecov]

Codecov Report

Merging #758 (6fe7c20) into main (e8a43eb) will decrease coverage by 0.00%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##             main     #758      +/-   ##
==========================================
- Coverage   93.20%   93.20%   -0.01%     
==========================================
  Files          27       27              
  Lines        2209     2208       -1     
==========================================
- Hits         2059     2058       -1     
  Misses        150      150              

Impacted Files	Coverage Δ
tedana/decomposition/_utils.py	0.00% <0.00%> (ø)
tedana/workflows/parser_utils.py	94.73% <66.66%> (ø)
tedana/combine.py	91.42% <80.76%> (ø)
tedana/decay.py	97.24% <84.21%> (ø)
tedana/selection/tedpca.py	83.33% <84.44%> (ø)
tedana/workflows/tedana.py	89.89% <89.56%> (ø)
tedana/decomposition/pca.py	86.51% <90.00%> (ø)
tedana/io.py	93.36% <92.85%> (ø)
tedana/reporting/static_figures.py	98.49% <93.54%> (ø)
tedana/selection/tedica.py	91.61% <94.73%> (ø)
... and 15 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a43eb...6fe7c20. Read the comment docs.

[tsalo]

I'm a little lost, since pre-commit hooks are a bit like magic to me. If you stage an incorrectly-formatted version of the code, what happens? Assuming you commit something that's not correctly formatted and open a PR to main, our linter CI will still catch it and fail, right?

EDIT: Also, what about "format on save" settings within editors, like this one for running black on save in VSCode?

[jbteves]

The issue is that what's staged and what's in the working area are two different things, so your commit will not necessarily contain something correctly formatted. Many pre-commit hooks use git itself to check for common errors, which can read the staging area specifically and will only tell you what needs to be done, not do it for you. However, black can't do that; it needs a full file path, and that doesn't exist for the staging area due to the internals of how git works. One thing that we could do is add a make line that would prepare things for commit, before anybody is staging things, like make formatted.

Again, one way around this is to use a full-fledged tool like pre-commit, where somebody much more experienced than me has figured out how to work things like this out.

I'm not sure about format on save settings in general, as I'm still using vim and kind of do my own thing. I'm happy to look up what needs to be done for VSCode since that's a pretty common editor these days and try it out. We could link to blog posts like that in the developer guidelines.

You can see discussion about this on the black repository itself, if you're curious.

[emdupre]

What about a set-up like this one ? https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/ Does that trigger the same commit fail issues ?

[jbteves]

I believe pre-commit handles that more robustly, however that would add pre-commit as a developer dependency. I believe this is the same post I read before commenting on this PR, actually 😆. Note that even with pre-commit, it will not automatically stage the changes for you since not modifying the staging area is one of the package's strong requirements.

[eurunuela]

I'm not sure about format on save settings in general, as I'm still using vim and kind of do my own thing. I'm happy to look up what needs to be done for VSCode since that's a pretty common editor these days and try it out. We could link to blog posts like that in the developer guidelines.

If you use EVIL mode (vim and Emacs) you can set it up to format on save.

I'm afraid I know nothing about pre-commit to add to the discussion. I format on save both on VSCode and Spacemacs.

[emdupre]

I believe pre-commit handles that more robustly, however that would add pre-commit as a developer dependency.

I'm ok with adding this as a developer dependency, since code-formatting will now be a bigger part of the development workflow ! Do you think its handling would make a reasonable workflow, at least ? I feel like failing a commit isn't viable.

[jbteves]

The only stumble is that pre-commit won't stage the changes, so if it fails then things will automatically get formatted but you'll still have to stage the changes that black and isort make manually. Personally, I actually prefer that. If we broadly agree on using pre-commit, though, I can work on adding it to the docs and making sure it's set up correctly and then add on to this PR.

[tsalo]

If pre-commit hooks are going to take a bit more time to figure out, should we just document our code style, resolve conflicts, and get this merged first, and then address auto-styling in a later PR?

[jbteves]

That seems reasonable. We should go ahead and add checks to the Makefile's lint so that black and isort checks are run, so that we know if a PR is formatted correctly automatically even if we can't apply the formatting automatically. I can add to this PR if you like.

Additionally, if I run black --check tedana from the root directory, I see that actually several files are still not passing black's checks:

would reformat /Users/tevesjb/repositories/tedana/tedana/info.py
would reformat /Users/tevesjb/repositories/tedana/tedana/decomposition/pca.py
would reformat /Users/tevesjb/repositories/tedana/tedana/gscontrol.py
would reformat /Users/tevesjb/repositories/tedana/tedana/reporting/static_figures.py
would reformat /Users/tevesjb/repositories/tedana/tedana/metrics/dependence.py
would reformat /Users/tevesjb/repositories/tedana/tedana/tests/test_metrics.py
would reformat /Users/tevesjb/repositories/tedana/tedana/metrics/collect.py
would reformat /Users/tevesjb/repositories/tedana/tedana/workflows/t2smap.py
would reformat /Users/tevesjb/repositories/tedana/tedana/workflows/tedana.py
Oh no! 💥 💔 💥
9 files would be reformatted, 38 files would be left unchanged.

[tsalo]

I added flake8-black and flake8-isort to the dependencies, so when we run flake8 in the CI it should flag any cases where black or isort would make changes. How would that interact with changes to the Makefile?

Additionally, if I run black --check tedana from the root directory, I see that actually several files are still not passing black's checks

That's weird... I'm not seeing those when I run black locally.

[jbteves]

I added flake8-black and flake8-isort to the dependencies, so when we run flake8 in the CI it should flag any cases where black or isort would make changes. How would that interact with changes to the Makefile?

Hm, maybe I'm misunderstanding something, but under the style check it seems to only be using make lint.

That's weird... I'm not seeing those when I run black locally.

Might black have a user config being applied that is more permissive? I don't have any local configurations, just what I pulled from PyPI.

[tsalo]

I think make lint just runs flake8, though, so it should still be using the flake8 extensions that are now part of our testing dependencies.

Might black have a user config being applied that is more permissive? I don't have any local configurations, just what I pulled from PyPI.

black should use the settings I put into pyproject.toml. I noticed that the CI isn't catching any issues though. Do you want to try running black locally and pushing here? That way we can see what your local run caught than mine didn't.

[eurunuela]

LGTM! Does the toml file fix linting errors on merge?

[tsalo]

The TOML file just defines the settings for black and isort that we want. We'll have to look into pre-commit hooks and other tools for anything automated.

[tsalo]

Since #795 introduces a number of style-based changes due to black and isort auto-formatting, I'd love to get this PR merged before that one. That way the style stuff will track back to a style PR instead of a content one. If anyone has time to review (especially the config files, rather than the reformatted code), that would be super helpful.

[tsalo]

Thanks @eurunuela!

@jbteves, would you be willing to review? Since we chose to push pre-commit hook information until a later PR, I think it would be fine for you to be a reviewer.

[handwerkerd]

No issues jumped out at me, the code runs, and I was able to get Black working in my IDE. I can't really review the specific configuration changes that set Black as the new standard. I know others have looked at that so your choice on whether to wait for one more pair of eyes to look at the configuration changes before merging.

[jbteves]

LGTM, thanks @tsalo ! At some point I'll see if I can figure out the easiest way to get it to run automatically for commits.

[tsalo]

Thanks all. Merging now.