Xarray: black formatting

[crusaderky]

I, like many others, have irreversibly fallen in love with black.
Can we apply it to the existing codebase and as an enforced CI test?
The only (big) problem is that developers will need to manually apply it to any open branches and then merge from master - and even then, merging likely won't be trivial.
How did the dask project tackle the issue?

[max-sixty]

I would be v keen. I find it _great_.
But I'm generally too keen on a) tools and b) changing everything at once, so I discount myself a bit here!

I don't think it's possible to apply incrementally, unfortunately (e.g. only on changed lines). So it would require a single sweeping change. Interested to know how other projects are handling. IIRC they're applying it to some parts of stdlib too.

[jhamman]

If we do this, which I am +1 on, we should add a pre-commit hook so we don't have to think about the manual applications. We should probably just copy the dask approach: https://github.com/dask/dask/pull/4983.

[shoyer]

+1 from me, too.

It's definitely going to cause some nasty merge conflicts for any work in progress, but hopefully it's straightforward to fix those by running black on the PRs and then merging in master.

We should be able to simply add another job on Azure for this. The lint task using flake8 on Azure's pre-installed Python runs very quickly (less than 30 seconds!) and I would expect to see the same from black.

[andersy005]

Since Black requires Python >=3.6, which would set the minimum Python version to 3.6 for contributing to xarray (which currently requires Python >= 3.5), are there any plans of dropping Python 3.5 support in the near future?

[shoyer]

Since Black requires Python >=3.6, which would set the minimum Python version to 3.6 for contributing to xarray (which currently requires Python >= 3.5), are there any plans of dropping Python 3.5 support in the near future?

I don't think these decisions need to be coupled. It's OK to make contributors upgrade to a newer version of Python before all our users.

There are no immediate plans to drop Python 3.5 support, but we haven't really discussed it either. I think it would definitely make sense to drop Python 3.5 after Python 3.8 is available, maybe sooner. Four major Python versions would definitely be too many.

[max-sixty]

I'm happy to do this today

[shoyer]

Let's wait a little while for the burst of activity from SciPy to die down
first.

On Mon, Jul 15, 2019 at 8:03 AM Maximilian Roos [e-mail]
wrote:

I'm happy to do this today

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/pydata/xarray/issues/3092?email_source=notifications&email_token=AAJJFVR2KNJP3PXHPG4ZZK3P7SGTXA5CNFSM4IA3BF72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ57I3A#issuecomment-511439980,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJJFVUQ4OMMUZJ5LQEFF7TP7SGTXANCNFSM4IA3BF7Q
.

[max-sixty]

@shoyer great

TODOs (please add where I've missed)

• [ ] Reformat code
• [ ] CI checks
• [ ] Short instructions for how to merge an existing PR (i.e. avoid manual merge resolution)
• [ ] Black badge
• [ ] Do we want to keep flake8 checks? Black is mostly stricter but not always, e.g. on lines at the end of files. (+0.3 from me to use only black and stop using flake8)
• [ ] Do we want to include isort? (-0.1 from me, even though I like the tool)

[crusaderky]

flake8 is not just about formatting; none of the pyflakes errors (e.g. double/unused imports) are covered by black.

I didn't know about isort, nice. However, from a very quick test, it's not compatible with black e.g. if you run black -> isort -> black, the two tools keep touching each other's changes.

[crusaderky]

I just ran isort on my own project.
Overall, it gave me a bunch of good suggestions, but the amount of times I twitched my nose and manually reverted what it did are too many for my tastes.

Besides the aforementioned incompatibilities with black, I hate when it changes

from .. import module1
from .. import module2

to

from .. import module1, module2

which in my opinion flies in the face of the PEP8 rule of never importing two modules on the same line.

So my opinion is: +1 for a manually vetted one-time run; -1 for automatic integration in CI.

[max-sixty]

I didn't know about isort, nice. However, from a very quick test, it's not compatible with black

There's a specific isort config that's compatible with black; this is what we use internally (there's a similar official version I believe):

[isort]
default_section=THIRDPARTY
force_grid_wrap=0
include_trailing_comma=True
line_length=88
multi_line_output=3
use_parentheses=True

[crusaderky]

I tried your setup.cfg above but still getting inconsistencies with black when I have comments in the imports.

black:

import foo

# Let me explain why we do this
import bar

isort:

import foo
# Let me explain why we do this
import bar

and I strongly agree with black.

[Zac-HD]

@max-sixty - closed by #3142 I think?

[max-sixty]

Thanks @Zac-HD