Black

[max-sixty]

From #3092

• Reformat code
• CI checks
• Short instructions for how to merge an existing PR (i.e. avoid manual merge resolution)
  • Not sure if there's magic here - I think people would just have to format their code and then hope git can resolve (i.e. because there would still be no common parent)?
• 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)

[max-sixty]

FYI, unfortunately pep8speaks doesn't yet support black

[max-sixty]

I think we can suggest something like this to existing PRs to eliminate manual conflicts, and I'm attempting to find something more official. Let me know if anyone knows of something:

• Merge master prior to the black commit (any conflicts are real conflicts and require resolving)
• Apply black . to this branch
• Merge master at the black commit, resolving in favor of our changes: git merge [black-commit] -X ours
• Merge current master (any conflicts here are real and again require resolving)

[max-sixty]

What are people's thoughts on doing this now? There are a few big merges happening atm, so could do this after those.

I tested the approach above and it worked well. I don't have any more external validation than my test though. If anyone has experience here, please let us know.

[shoyer]

Let's do it after merging #2277

…

On Mon, Aug 5, 2019 at 11:17 AM Maximilian Roos ***@***.***> wrote: What are people's thoughts on doing this now? There are a few big merges happening atm, so could do this after those. I tested the approach above <#3142 (comment)> and it worked well. I don't have any more external validation than my test though. If anyone has experience here, please let us know. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3142?email_source=notifications&email_token=AAJJFVVE7WXUFCD4PTPJ3W3QDBVCVA5CNFSM4IE5PZH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SUOXY#issuecomment-518342495>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJJFVS5ZVHUHZKHTRBZTJTQDBVCVANCNFSM4IE5PZHQ> .

[Zac-HD]

Sadly if there's a way to automatically fix up merge conflicts I don't know of it 😭. However I'd be happy to help anyone who is having trouble rebasing their open PR.

On a different note, have you considered adding the pyupgrade and autoflake8 autoformatters? They go further than Black in that they do change the AST, but it's a nice way to ensure consistency of style, automatically remove unused imports and variables for flake8, and so on.

[crusaderky]

I did a manual pass of pyupgrade (#3190), and it looks good.
+1, but let's avoid bloating and leave it for a later PR

[max-sixty]

On a different note, have you considered adding the pyupgrade and autoflake8 autoformatters?

Nice, I didn't know those. I had use autopep8 before. isort is another I use a lot.

+1, but let's avoid bloating and leave it for a later PR

Yeah - not sure of the balance here.
I think the focus should be on easy initial contributions. Consistent formatting is helpful in reading and reviewing code, and eliminating any formatting iterations.
Each extra tool's benefit needs to be weighed against the startup cost of another install - unless there's a way of eliminating that (e.g. I've seen some bots add the code changes from GH)

An alternative is to run those additional tools across the whole codebase on occasion, assuming this doesn't create too many merge conflicts.

Interested in others' thoughts... (but probably for another PR)

[max-sixty]

@pydata/xarray this is ready to go!

Any final comments / thoughts / feedback?

[dcherian]

❗

[max-sixty]

OK - going to hit the button as soon as tests pass.

🙏

[max-sixty]

Please ping here / tag me on the relevant PR with any issues, and I'll help!