Thoughts on Implementing black and isort in MetPy

[jthielen]

Based on xarray and pint at least, there seems to be some momentum in the ecosystem towards automatic code formatting with black and isort, and after some initial uncertainty, I've started taking a liking to it myself. Would this be something worth pursuing in MetPy? @dopplershift IIRC, you've had some dissension with black's formatting choices, but I thought it would still be worth discussing.

xref hgrecco/pint#929

[dopplershift]

🤨

I definitely have misgivings about how black formats some code. isort seems configurable enough, so that’s not an issue. I think first though, it’s best to step back first and ask: what problem are we trying to solve? Is it lessening the burden of correcting formatting lint?

Were you thinking of having this run by default as a git pre-commit hook?

[jthielen]

I think first though, it’s best to step back first and ask: what problem are we trying to solve? Is it lessening the burden of correcting formatting lint?

Yes, it is firstly that. After working with black for a little while, I've realized how much time I spend making linting pass when working on MetPy...not enough to be a complete hindrance to my work, but definitely enough to be annoying. Also, it removes the burden of deciding how to best make flake8 happy...or of reaching barely passable formatting and giving up (Exhibit 1: https://github.com/Unidata/MetPy/blob/v1.0.0rc1/src/metpy/xarray.py#L842-L868).

However, there is also the argument of consistent formatting across code bases in the ecosystem, which I think is a laudable goal. There is something to be said about "Blackened code looks the same regardless of the project you're reading. Formatting becomes transparent after a while and you can focus on the content instead."

Also, this is me preemptively getting the conversation started before something like

@exporter.export
@preprocess_xarray
@check_units(
    "[length]",
    "[speed]",
    "[speed]",
    "[length]",
    bottom="[length]",
    storm_u="[speed]",
    storm_v="[speed]",
)
def storm_relative_helicity(
    height,
    u,
    v,
    depth,
    *,
    bottom=0 * units.m,
    storm_u=0 * units("m/s"),
    storm_v=0 * units("m/s")
):

slips into one of my PRs due to new habits. 😉

Were you thinking of having this run by default as a git pre-commit hook?

Not so much as run by default as a pre-commit hook, but with a CI check, PR checklist item, and optional pre-commit hook like Dask and xarray (e.g. http://xarray.pydata.org/en/stable/contributing.html#code-formatting).

[eliteuser26]

I was just reading about this the other day explaining the good and bad things about black and isort.

[dopplershift]

@jthielen I'll have to think it over some more, but I'm leaning towards at least allowing it. Is there a way to run it in some kind of "diff" mode so we don't have to do a massive rewrite of files?

[jthielen]

Is there a way to run it in some kind of "diff" mode so we don't have to do a massive rewrite of files?

If I'm interpreting this correctly (only auto-format the changes made in a given git diff), I think the answer is no. While I think it would be neat to have such an option were in place for incremental adoption, the README states:

Black reformats entire files in place. It is not configurable. It doesn't take previous formatting into account. It doesn't reformat blocks that start with # fmt: off and end with # fmt: on.

[eliteuser26]

Also for isort it was indicated to be careful with this module as it might screw up the list of from and import at the beginning of file. Python is quite finicky about the placement of modules as it has to import in the right order for some modules to work without giving an import error. This has to do with dependencies. This is also flagged in flake8. This happened to my code.

[dopplershift]

MetPy has already been running with a lint check to alphabetize and group imports, just like isort has done. It hasn't caused any problems that I'm aware of.

In general, if you're having import order issues, in the overwhelming amount of cases (but not all) this means something isn't working right. This sometimes indicates a mismatch/incompatibility in what library things are compiled against.

[eliteuser26]

It only happened in rare cases where an import was needed before other imports. Maybe this has been fixed over the years. Didn't see this happen for a while now. Only in a few cases where flake8 flagged it. Since correcting this it didn't happen afterwards.

[akrherz]

I love providing my unqualified two cents here, so here I go again :)

When I first started using black, I was skeptical and did not like what it was doing to my code formatting. I've come now to not care about the formatting that black enforces, which is what I think the point of the tool is. Just enforce some standard and move on to more important things like actually getting rid of bugs. I just write the code, let the pre-commit black hook do its thing and git commit the change.

With regards to isort, I've had inconsistencies with using it and omitted it for now.

[MinchinWeb]

In the spirit of Hackotoberfest, if I were to take this one, would it be appreciated?

[dopplershift]

@MinchinWeb somebody running it and submitting it so I can see what horrors it does to our codebase would be fine and appreciated (and accepted for hacktoberfes--don't forget to set the line length to 95). Whether it gets merged would depend on how much it offends my delicate code sensibilities. 😉

[MinchinWeb]

@dopplershift

Whether it gets merged would depend on how much it offends my delicate code sensibilities. 😉

For some reason, this cracks me up :)

#1530 is just isort, #1531 is both isort and black (black would mess with your imports a little too, so it seems to me that if you do black, you should do isort too. And this way, hopefully there are no merge conflicts if you pick up both). Black mostly seems to be replacing quote marks (single quotes to double quotes) and adding lots of space to matrices.

As a side, because black adjusts the code so significantly, almost any other merged code is going to cause painful merge conflicts, so if you like what you see, I'd suggest merging it in right away.

On the Hackotoberfest front, they are only "accepting" Pull Requests this year "by merging it, adding the 'hacktoberfest-accepted' label to it, or [a maintainer] approving it." So if you would add the label (hacktoberfest-accepted) while you decide whether (or "weather", heehee) your code sensibilities are offended, that would be appreciated 😉

[dopplershift]

@MinchinWeb Thanks and happy to.

[dopplershift]

Capturing some more thoughts on the absurd formatting decisions that black makes.

[dopplershift]

So one year later, while I still hate many of black's formatting decisions, we're quickly reaching the point where having NO autoformatter is not an acceptable choice for a good Python project. So if I can't figure out how to make YAPF do what I want, we'll just need to adopt black (or blue).

[eliteuser26]

Even though I don't agree with some decisions that are being used for formatting code, I started using it as suggested in part of the code and it doesn't seem to create any problems at the moment. I do put imported modules (meaning not together) on separate lines and it seems to work fine. As for function parameters, I put parameters on separate lines when it doesn't fit on one line. I think it is just a matter of preference. However I still use a line length of 95 characters as suggested for Metpy.