Add black as a pre-commit hook

[betatim]

This adds black as a pre-commit hook.

I followed the link to the blog post Joe linked to and then ended up on https://github.com/ambv/black#version-control-integration which uses the same tool (pre-commit) to set things up. Conclusion: I just followed the instructions 😄.

I didn't add flake8 as a pre-commit hook as I think black will reformat changes you made that would violate pep8 in black's steamroller way of dealing with things.

Closes #132

[codecov-io]

Codecov Report

Merging #158 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #158   +/-   ##
=======================================
  Coverage   70.57%   70.57%           
=======================================
  Files          11       11           
  Lines         554      554           
=======================================
  Hits          391      391           
  Misses        163      163

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e757348...4879912. Read the comment docs.

[betatim]

One thing that is a bit weird with this setup is that if you edit a single line in a file then black will reformat the whole file. This means your single line edit becomes huge. Checking if it is possible to configure black so that it only applies its reformatting to the lines you edited (or nearby).

That way you can slowly convert a file to the new style as you anyway edit stuff. Or maybe we make one PR that applies black to everything. I like keeping commits that do nothing but reformat separate from commits that change contents :-/

[lwasser]

hey @betatim i do agree that it would be harder to have to review a code update that is a few lines with more lines of edits given formatting fixes via black. i'm definitely liking black more and getting used to seeing code formatted that way. It also means i don't have to ping people about syntax which is great :) and no fighting with travis. So if there is a way to keep format edits separate that would be ideal.

or like you said we just run black on the entire repo first. i have it hooked into pycharm now so it's part of my workflow.

[betatim]

Did a bit of searching around but couldn't find anyone wanting to only format "changed lines". Maybe it is hard to do? For example if I add a new argument to a function then just formatting that one line "according to black" would create pretty weird looking code if the rest of the arguments were still formatted the old way :)

Right now I am thinking: if there is consensus to go for black I'd make one PR that does nothing but run black on the whole project and merge that. After that has gone in having a commit hook like the one proposed here wouldn't create extra noise.

[lwasser]

I believe that we have a consensus here. is that right @mbjoseph i just see your thumbs up :) if so i think we can merge this!! i'm using black more and more. it's just very easy to manage.

[mbjoseph]

I think adding a pre-commit hook for black is a great idea - if I am reading @betatim's suggestion correctly:

Right now I am thinking: if there is consensus to go for black I'd make one PR that does nothing but run black on the whole project and merge that. After that has gone in having a commit hook like the one proposed here wouldn't create extra noise.

...then we want to wait to merge this until we merge a separate PR that incorporates all of the formatting changes that black will make in the existing code. But once we merge in the formatting changes, I think we are good to merge this!

[lwasser]

oooh my apologies. you are totally right @mbjoseph i misread this. I think that is a great idea. i've been running black on every file as i work in pycharm but you can do an entire directory easily too at the CL. good call.

[mbjoseph]

Ok now that #161 is merged, we should be good to go with merging this PR. I have just one quick question though - do we need to add black in the dev-requirements.txt file?

[betatim]

Before you hit merge: the black default is 88. Which is probably what got used in #161

I think if you aren't as stuck in your ways as I am 88 is a good choice (and simplifies things).

[lwasser]

oh... you know i had thought i reran it using 79 but you are likely right @betatim
@mbjoseph do you have thoughts on 79 vs 88 character line width? pycharm defaults to 88 with black. i'm still trying to figure out how to fix that. should we stick to pep8 convention? black actually does break a few things like the single vs double quote rules already. thoughts??

[lwasser]

ok i did fix this to be 79 last night @betatim @mbjoseph -- in my last commit here

we are in agreement that we'd like to use 79 for this repo (just chatted with max) SO... can we merge this?! i'm excited to have everyone apply this on their pr's and such too.

[lwasser]

@mbjoseph that is a good question. i just looked at that file and it seems to only have reqs for docs. But i would think we want to have black as a requirement somewhere i'm just not sure where it should be. maybe there what is protocol here?

[lwasser]

so stealing this from @mbjoseph

[betatim]

do we need to add black in the dev-requirements.txt file?

I think the precommit hook package does some magic to take care of installing black for the purposes of the pre-commit hook (in a different temporary environment??). It "just worked" for me.

[mbjoseph]

Edit: I now have this working locally! I needed to install pre-commit through conda, and not via pip. 📦 🔥 🐍

[lwasser]

super duper weird. this all works fine on my computer @mbjoseph . (MAC OS)

[mbjoseph]

I think it would be good to add instructions to CONTRIBUTING.rst for folks to execute pre-commit install, but we can do that after merging this...