Adding pep8 compliance test case?

[kazamatzuri]

Since this came up in the discussion, would it be a good idea to just jump into the deep and make add a test to ensure pep8 compliance going forward?
Do people have a strong opinion about pep8 settings or should we just go with the vanilla version for now?

Thoughts?

[twosigmajab]

Consider an automatic code formatter like black instead? (You can similarly configure a test that fails if the formatter had to make any changes to the code.)

[barakalon]

+1 for black
Should be trivial to add such a test to CI: https://github.com/plangrid/flask-rebar/blob/master/.travis.yml

[kazamatzuri]

Sounds interesting, I've never used black before. It's still in beta and i'm unclear how we'd enforce its usage cross the board by everyone other than having a test in place anyways?

My understanding is:

• A test will enforce pep8/pycodestyle compliance at check-in
• Black is a tool that will allow the contributors to easily conform to those constraints (by doing the work for them...)

Is that about right? In that case we could add the test and leave it up to people how to make their code pass (i.e. using black,yapf, or autopep8)

[twosigmajab]

Here's an example with yapf: https://github.com/python-trio/trio/blob/master/check.sh
That script gets run as part of the project's CI. The script runs the formatter such that if it makes changes, it exits nonzero, failing the build, which in turn blocks merging of any associated PR.
I wouldn't be concerned about black being still in beta (and you can see python-trio/trio#768 for more on yapf vs black), but whichever one the maintainers want to adopt shouldn't be too hard to get working.

[kazamatzuri]

True, I tried a couple of them. I still think the validation should be part of the tests, how people get to the compliance should be left to them: black,yapf,autopep8 (there are probably tons more), maybe people have preferences.

[barakalon]

@kazamatzuri black is "uncompromising", thus I think it's all or nothing with black.

IMO, having to manually adjust code to pass a linter is a nuisance. And I imagine that configuring an autoformatter to align perfectly with a linter will be similarly painful.

That's because even with flake8, there's still wiggle room. And while it may eradicate formatting comments from PRs, it still leaves some formatting decisions up to authors.

Black is unlike the rest in that it aims to make the format of a file deterministic. Authors don't have to think about how they will be pep8 compliant - those decisions are automated. Tools like black and prettier are becoming increasingly popular, and I think for good reason.

But with that said, I haven't used black for any extended period of time. So I'm not that firm in my opinions here.

[kazamatzuri]

Ok. Assuming that's the consensus then, how should we go about enforcing it?
Do we want it as pre-commit hook?

[kazamatzuri]

Just to test it out, I gave this a go. This is going to be one huge diff.

https://github.com/kazamatzuri/flask-rebar/tree/enforcing-black

Between forcing black to run as pre-commit hook, and actually running it, it's touch pretty much all python files and a couple more..
Of course, the hope is, once we have this in place, all the formatting discussions are going to be gone.

[juliusiv]

To chime in, I implemented black in a project at PlanGrid a month ago and it's been pretty nice. It's setup to run as a pre-commit hook so you don't really need to think about it. I also hooked it into our build, so Jenkins runs black on the whole repo on a merge and fails if the output is non-zero; the only time you should really get an error from that build step is if you haven't setup the repo correctly. It's been so great not to think about formatting anymore.

@kazamatzuri that PR looks good, the only thing I would add is a Travis build step like the one I mentioned. I've never written a Travis step before but if you don't have the time I can give it a shot.

[kazamatzuri]

@juliusiv I'm in the same boat, I've never written a travis step for that. If you have one already in that other project it's probably easier for you to replicate that here than for me to figure it out. Plus I don't have a travis instance ready to test things.
So you folks think I should make a PR with the above?

[juliusiv]

@kazamatzuri I want to add the travis build step if we're going to enforce black but go ahead and open the PR and I'll just add to it. Sound good?

[kazamatzuri]

With having #68 in now, we're all set here. Thank you for your time!