Going black (i.e. adopting an _opinionated_ python code formatter)

[smoia]

I was already thinking about proposing it, but then @tsalo applied it on his #219 so it's a good moment to propose it.

In the line of automating all that is automatable, there is one more thing we should adopt, a code formatter.

Code formatters are scripts that chew our code and our PRs and spit out a standardised formatted version of the code, as long as the code doesn't contain big mistakes. They shouldn't change radically the code, rather adjust it with simple rules like "always use ' instead of " for strings" or "always adopt a certain indentation rule in your scripts". "Chill" code formatters just check that a coding style is respected (e.g. autopep8 checks that pep8 is respected), while there are "opinionated" code formatters that format the code even if it respect a standard (in our case, pep8).

One (apparently) popular opinionated python code formatter is black. It's very strict and either we agree with its rules or we don't.
Another opinionated code formatter is YAPF. It's less strict, in the sense that it allows for some customisations, but it does a similar job.

There are pros and cons in adapting a code formatter.
The biggest pro: our code will always be formatted in the same way, independently from its author, so we can forget comments like "please reformat following our style" in reviews and other minutiae.
Cons: If we ask the commits to be already formatted, contributors needs to run black locally before a PR. Alternatively, if we run it during a PR merge (we need to understand how to do it smartly), the code that we will see in the repo might not be exactly the same that we wrote - although it will be very similar.

@physiopy/all, what do you think about adopting an opinionated code formatter? And what do you think about adopting black?

Outstanding questions

1. Should we adapt an opinionated code formatter?
2. If so, which one between black and YAPF?

PS: Labelling this urgent because we might need to wait for this before merging #219 - and we need to do that in the upcoming month. Deadline is next dev call (remember that next month it will be during the first week).

[smoia]

(Personally, I don't love black because I prefer using ' over " in strings - I find it more readable. However, I can adapt to a life of " if we decide we like Black so much).

[tsalo]

I had similar reservations about black (and for the same reason), but I've adapted and adopted it for NiMARE. It vastly improves readability across programmers (not so much within programmer, but that's going to be the case for any code formatter). I haven't played with YAPF, but I use black and vote for it.

I'd also recommend using isort to automatically sort imports alphabetically and based on the three groups (builtins, third-party, then local).

[eurunuela]

I'm happy with black and isort even if I don't like writing my strings with " 😅

[62442katieb]

I feel the same! (both about " vs ' and about black)

[smoia]

I thought the ' vs " was an OCD moment of mine, glad to see it's not (or it's a shared OCD moment).
I have three options then:

1. We momentarily run black on the repo, while we dedicate a little part of the brainhack setting up YAPF for our repos, or
2. We set up YAPF ASAP (volunteer needed) so that we don't stall Add multi-run workflow synchronized with BIDS dataset #219 any further and @tsalo can run it before our review, or
3. We accept our human limitations, adopt black, and use our ' in the rest of the code. We can still decide to switch to YAPF later on.

I vote 1

About isort, in this moment we're following an internal convention for which we respect pep8, but we list all the from imports after the normal ones. Can we set up isort to do the same?

[tsalo]

It's easy enough to just incorporate black into your CI. You just need to swap out flake8 with flake8-black in your dependencies, and then calling flake8 in the CI should use the black version automatically. See here.

isort does the following:

import builtin1
import builtin2
from builtin1 import module

import third_party
from third_party import module

import local
from local import module

I think that fits with what you're saying.

[eurunuela]

I'm happy with @tsalo 's suggestion.

[smoia]

@tsalo would you be able to set up the PR?
(if so, please assign this Issue to yourself)

[tsalo]

Will do!

[smoia]

Thanks! Once you open it, you can label it internal and assign the PR to me - I'm going to MRev it!

[RayStick]

Sorry for the slow response - this all sounds fine to me as long as there is some guidance/documentation on how to incorporate black locally if that's what we go for. 😅 Not that I am doing that much Python coding but one day ...