MAINT: Use black

[larsoner]

black seems to be where the community is going -- NumPy is planning on using it I think (?), and from a quick look it looks like pandas and scikit-learn already do or are headed in that direction. I'm sure @agramfort has opinions/experience to share here :)

It's possible to make a single big commit that makes all the changes, then add this to a git blame ignore file, which removes the potential blame pollution which (to me) was a big barrier to adoption.

So overall +1 for moving to black from me using whatever pre-commit hooks or whatever pandas and scikit-learn have done (despite me actually not particularly liking how black code ends up looking sometimes!).

I can volunteer to help people rebase or merge PRs that are broken by the changes if they need help.

cc @cbrnr who mentioned wanting longer than 79 char line length, this would be one way to do it.

EDIT:

• https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/
• https://github.com/scikit-learn/scikit-learn/blob/main/.git-blame-ignore-revs

[welcome]

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

[cbrnr]

Thanks for opening your first issue here @larsoner 😆!

I'm reluctant to go all Black. Of course this alleviates us from all style-related issues, but on the other hand we sometimes want to use custom formatting to improve readability. This is just not possible with Black.

My vote is to just allow a line length of 92 characters for new code, so no changes are necessary for existing code. Of course, people are then free to rewrap a whole file as part of some other change.

[agramfort]

you're so European minded Clemens :) I would say it's either 0 or 1. Either we go with Black and follow what sklearn / numpy do or we don't change. let's not be exceptions here.

…

Message ID: ***@***.***>

[cbrnr]

you're so European minded Clemens :)

I hope this is a compliment 😄?

If you really want all or nothing, I'm going to vote against Black.

Can we maybe still do a quick poll just to see what others think?

1. 😄 Leave everything as is (79 chars hard limit with # noqa as a last resort)
2. 🎉 Use Black
3. 🚀 Just increase our maximum line length to 92 characters

[larsoner]

+0 on 1
+1 on 2
-1 on 3

[agramfort]

+0 on 1 +1 on 2 -1 on 3 Message ID: ***@***.***>

…

[mmagnuski]

I'm not familiar with Black but I have a vague recollection that I didn't like some of the formatting rules it applies. What would be the benefit of switching to Black (apart from the fact that it seems to be a trend now)?
Currently I'm +1 on staying how it is.

[cbrnr]

@mmagnuski the benefit is that Black auto-formats everything. The issue is that Black auto-formats everything. So there is no in between if for some reason (readability) you would like to have some custom formatting.

[agramfort]

standards are always a middle ground and a compromise. If you deviate from the standard then it's useless and you are back to exceptions and it can do more harm than good. Accept the standard as it is or don't consider it.

…

Message ID: ***@***.***>

[cbrnr]

Black is not a standard, it's an opinionated formatter. Line lengths can be set even in Black (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length), so in my opinion we could set only a new line length (to 92 or 88). I'm happy with our style, it's just the line lengths that bother me.

[mmagnuski]

@cbrnr @agramfort
I'm also happy with our style (much more than what Black does). And I'm happy with current line limit - @cbrnr when is it an issue?

[cbrnr]

when is it an issue?

Everytime. It's just not enough characters, I almost always have to break it up into multiple lines.

Most recent example: https://github.com/mne-tools/mne-python/pull/10638/files

With 92 characters, the line could have stayed one line:

ch_data = many_chunk[:, ch_offsets[ci]:ch_offsets[ci + 1]].copy()

Now it's this:

ch_data = many_chunk[:,
                     ch_offsets[ci]:ch_offsets[ci + 1]].copy()

[mmagnuski]

I'm neutral on changing the line limit - I rarely encounter such edge cases. And I'd be fine with:

ch_data = (
    many_chunk[:, ch_offsets[ci]:ch_offsets[ci + 1]].copy())

in this particular case. :)
But I understand it can be a bit frustrating at times, when you are forced to perform line-length exercises.

[sappelhoff]

I am in favor of black. I have adopted it for most of my projects and the few times that I have run into issues are heavily outweighed by the consistency, readability, and "ease of reading across projects" that black provides me with.

The issues that i have run into are furthermore typically minor: For example "I want this list to be in one line!" and then black puts it into multiple lines. I can really live with that.

I also use black together with isort to properly sort imports and with flake8 + flake8-docstrings to enforce a reasonable docstring formatting (numpy format).

[hoechenberger]

Everytime. It's just not enough characters, I almost always have to break it up into multiple lines.

+100 on this. 79 chars are just not helping anyone anymore these days. If your display is not wide enough, just enable soft-wraps, but leave the rest of us in peace ;)
No but all joking aside, let's decide to make lines longer. Please.

Also I'm +999 on using black and auto-formatting all new code using a GH commit hook or whatever. I'm so tired of CI telling me I need to reformat stuff because I forgot something again.

Re not liking some of black's rules:
Yeah but so what, at least it will be more consistent than what we're having now :)

[hoechenberger]

btw Black and Isort have just recently landed in VS Code as "stand-alone" language server protocol plugins. It's super nice and easy to use them both, and doesn't require any installation into your Python environment(s):

https://devblogs.microsoft.com/python/python-in-visual-studio-code-may-2022-release/#black-extension

[mmagnuski]

I must say I am leaning towards Black (just a little bit) as CIs remind me I forgot something again. But sill reading stories of how Black can rearrange a matrix into 400-line snake monstrocity gives me goosebumps. :)

[drammock]

Just to be clear about what I think the proposals are:

1. increase our max line length. sure, +1
2. Adopt black (with some suitable, possibly non-default configuration), run it on the codebase, and afterward require all subsequent PRs to conform. I am +1 on this
3. do as in (1), and also use black in a GitHub commit hook. I am -1 on this and would need to be convinced otherwise / it would depend on the setup and user experience. To give an example of something I don't want: if we're using pre-commit I do not want it to automatically run black every time I make a commit locally (for big changes, I often make dozens of small commits and then rebase/squash later to make the commits more semantically coherent; running black on every small commit and on every rebase is a waste of time and processing resources). In contrast, if it's up to the user to run black and we have a CI or pull request bot that checks whether the changes conform to our black config... that would be fine with me but it doesn't fix the complaint that @hoechenberger and @mmagnuski are making (being annoyed at having CIs fail because they forgot to run flake or black or whatever). So... is there a third way? Is there something like pre-commit that will only run when you push instead of when you commit?
4. also adopt isort +1

[cbrnr]

@drammock we can have a pre-commit on GitHub, but this doesn't mean that you have to use it locally. I think it's not the end of the world if CIs fail because of style issues – we could put our style job before all other tests, which can depend on its successful completion. If style fails, no tests are run and the user can push style fixes.

So it seems that most of use are fine with increasing our max line length and isort, but we can do that without adopting black, which is still my preference.

[sappelhoff]

But still reading stories of how Black can rearrange a matrix into 400-line snake monstrocity gives me goosebumps. :)

I think there are solutions for that like:

• using a trailing comma or
• # fmt: off and # fmt: on wrapping that part of the code

https://stackoverflow.com/a/58584557/5201771

Black is not a standard, it's an opinionated formatter

an opinionated formatter that tons and tons of people use and that's also officially hosted on the org of the Python Software Foundation (https://github.com/psf/black) 😉

@cbrnr I understood your main argument against black is

we sometimes want to use custom formatting to improve readability. This is just not possible with Black.

do you think these situations outweigh the situations where our custom formatting currently makes readability worse?
(especially "cross project readability")

[cbrnr]

an opinionated formatter that tons and tons of people use

I still think there are more people not using it.

and that's also officially hosted on the org of the Python Software Foundation (https://github.com/psf/black) 😉

That's not a good argument, especially not for MNE-Python. We recommend conda instead of pip, which would be the official tool hosted at PyPA (OK, it's not PSF, but it's basically the same thing).

do you think these situations outweigh the situations where our custom formatting currently makes readability worse? (especially "cross project readability")

I have not used Black extensively, so my experience is limited. I'm using a style which is already pretty close to Black, so maybe y'all are right and we should do it. It's probably really a lot better than our current style.

[mmagnuski]

I looked more into what Black does and I think I'm fine with it. Things I don't like so much I can get used to. I'll test the vscode extension and will likely use it locally.

[agramfort]

we discussed during the MNE office hours.

I agreed that switching black is a way forward, although sticking with default black config.

please put your 👍 below this comment if you are on board

[mmagnuski]

Ah, I keep forgetting about the MNE hours. I will try to join you next time!

[drammock]

Is there something like pre-commit that will only run when you push instead of when you commit?

To answer my own question, apparently you can put any executable file into .git/hooks/pre-push so presumably we could set it up so that black runs before pushes, and aborts the push if the check fails.

[drammock]

just a note that after we do this, we should also add a .git-blame-ignore-revs file with the commit hash of the black-and-isort commits. See here for explanation:

https://akrabat.com/ignoring-revisions-with-git-blame/

There may be a few commits in the history we would want to add to that file as well? Not sure, sometimes big moves end up squashed in along with real code changes.

[larsoner]

.git-blame-ignore-revs file

For a template to follow we can look at numpy/numpydoc#394 which IIRC was done following what NumPy did

Not sure, sometimes big moves end up squashed in along with real code changes.

Yes this will be true for the last 2-3 years. But there are probably some before that that we could include, though

I'm thinking this could be an early PR in the 1.2 cycle

[cbrnr]

Does anyone have time in the near future to tackle the switch to Black formatting? Once this is implemented, I can enable Black as a pre-commit hook in #11541.

[larsoner]

It would be nice to get #11152 in first if possible since it's big. Maybe #11064, too, but since I'm authoring that I'm not as worried about the rebase/merge. But I have this marked for 1.4 so it should hopefully get done within the next month.

[cbrnr]

Could/should we start using Black style in PRs even before the big one is merged? It wouldn't hurt, right?

[larsoner]

Yes people can start doing that if they want

[agramfort]

please don't use black in a PR that changes things. I would prefer to have only reformatting in one PR so I don't have to scratch my head when reviewing.

…

Message ID: ***@***.***>

[cbrnr]

I only meant in new code. But it doesn't really work anyway because we're changing our line length from 79 to 88.