Adopt Black and isort

[tsalo]

Closes #324.

Proposed Changes

• Change flake8 dependency to flake8-black. Calling flake8 in the CI will use flake8-black automatically.
• Ignore E203 (whitespace before ':'), which flake8 flags, but which is actually not PEP8 compliant. See here for an explanation.

[tsalo]

There are surprisingly few style issues, which is nice to see!

@smoia, do you want to use isort as well? There's a flake8-isort plugin that could be added to the dependencies, and I think we'd need some isort-specific settings in setup.cfg to fit with black.

EDIT: Given that we don't want to let PRs sprawl, I think it would be good to open a separate PR (to be merged before this one) in which black is run on the whole repository. WDYT?

[eurunuela]

Thank you @tsalo ! It's great to see we were already close to the style required by black.

EDIT: Given that we don't want to let PRs sprawl, I think it would be good to open a separate PR (to be merged before this one) in which black is run on the whole repository. WDYT?

I'd open a separate PR.

[smoia]

Ok, one quick thought about this.
I thought we were going to add a CI job to run black during PR merge.

This solution obliges the user to run black locally instead. I don't love the idea, mainly because it complicates (a little, but still complicates) the process of contributing to the repos - it might scare contributors away (especially new ones) or force them to adopt black during coding. Can we check if setting up a CI action (actually, adding a job in the autorelease GH action would be better) is feasible?

[tsalo]

I considered using the black GitHub Action in NiMARE, but I was wary to give an Action the ability to directly commit to folks' pull requests. I chose to just use a stricter linter in that situation. It doesn't require contributors to run black (they could just fix things manually if they wanted), but it does flag poorly formatted code.

That said, it's up to you. I can try switching things out with an Action, but I honestly think you'd be the best person to set it up. I don't use Actions much (if at all), and the last time I tried it messed up my local repo.

[smoia]

I see - and understand the warning you're raising.
This might be worth discussing between developers (next meeting or back in #324).

I don't love to force users to adopt opinionated code formatters (or even worse a very opinionated code style) locally, honestly.
This is because I'd be the first to be pushed away if I had to.
Since I'll hardly adopt double quotes or line splitting during coding, I know I won't adjust things manually. I can run black before readying a PR for review, but if I need to work on the code again after a review I would be disoriented, making the process a bit more bothersome. I might even end up committing, running black and re-commiting. Uff (and I know a couple of physiopy contributors that might be even more exasperated by this).
Besides, new users might already overcome the challenge of understanding the library, (possibly) learn and code in python, install or run an easy-peasy linter (still problematic for some of the more expert contributors), possibly add documentation and tests. Asking to adopt a strict format or run black might be a bit too much. Especially since we're not talking about unreadable vs readable code - IMMO "simple" flake8 is not a poorly formatted code, and going through #328 I find blackened code less clear than the previous version (I know it's a matter of habit).

However, if @physiopy/all prefer to run black locally before PR, we can do that - let's keep the discussion flowing.

[eurunuela]

However, if @physiopy/all prefer to run black locally before PR, we can do that - let's keep the discussion flowing.

I'd rather have the linter tell us what to change. I think asking contributors to run black locally is a bit too much. I think we should make our lives easier and not complicate our contributions too much!

[smoia]

What if we discuss this in the upcoming meeting?
We can start the agenda now, given that it's next week.

[tsalo]

Where did we land on this? Should I close?

[smoia]

Sorry @tsalo, I've been pretty busy with the rest of my commitments. I need just a bit more time to work on this!

[smoia]

Hey @tsalo!
Would you still be interested in following the black-ification of the library?
If so, the library changed a little, so could you rebase on the latest master?

After that, we can implement pre-commits to automatically run isort and black on the files.

[tsalo]

I'm happy to give it a try. I'll try to fix up this PR later today.

[smoia]

Thank you!

[codecov]

Codecov Report

Merging #327 (ac03c5c) into master (d3a7443) will increase coverage by 0.17%.
The diff coverage is n/a.

❗ Current head ac03c5c differs from pull request most recent head e3496d7. Consider uploading reports for the commit e3496d7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #327      +/-   ##
==========================================
+ Coverage   94.25%   94.42%   +0.17%     
==========================================
  Files           8        8              
  Lines         974      879      -95     
==========================================
- Hits          918      830      -88     
+ Misses         56       49       -7     

see 6 files with indirect coverage changes

[smoia]

Let me know when you want me to review to merge!

BTW, I'm going to configure Pre Commit on all Physiopy packages, so if you want to go ahead and add it here too, I'm following this configuration with this setup from our repository template

[tsalo]

@smoia I added the pre-commit config, but I've never used pre-commit before. Is adding the config file enough?

[smoia]

Hey @tsalo! On your side it should be sufficient, although you may want to activate it locally. On my side, I just allowed pre-commit to run on phys2bids.

pre-commit.ci run

[smoia]

And now it's running!

[smoia]

Hey, I trust you're checking the results of the format.

If I can ask you, there's a couple of things we didn't want to automatically format. There are also a couple of issues with strings and if you could set it up to ignore configuration files, that'd be great!

[smoia]

If I remember correctly, we wanted not to auto-format the two aliases dictionaries

[smoia]

Oh, and we're not doing checks on tests, at least for docstrings.

[smoia]

@tsalo I tried to update this PR in order to merge it, but I get an error I can't figure out on the style check.

Locally, isort and black skip all files. Remotely, something isn't happy.

Do you have ideas on why?

[smoia]

Ok, problem found. It was "just" a black version issue.