Add pre commit hooks

[dnephin]

This adds http://pre-commit.com as a linting step, in place of just flake8 (flake8 is still there as one of the steps).

All checks will be run as part of CI, but they also get installed as a pre-commit hook in your local repo. So instead of waiting for the build to run (and fail), you get notified of errors before you can even commit.

Docs for the hooks are here: https://github.com/pre-commit/pre-commit-hooks and https://github.com/asottile/reorder_python_imports

The second commit is all the files with fixed whitespace and import ordering.

cc @mnowster since you expressed some interest in keeping imports sorted

[dnephin]

Output from jenkins

[INFO] Initializing environment for git://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for git://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Initializing environment for git://github.com/asottile/reorder_python_imports.
[INFO] Installing environment for git://github.com/asottile/reorder_python_imports.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Check for added large files..............................................Passed
Check docstring is first.................................................Passed
Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Flake8...................................................................Passed
Tests should end in _test.py.............................................Passed
Fix requirements.txt.....................................................Passed
Trim Trailing Whitespace.................................................Passed
Reorder python imports...................................................Passed
___________________________________ summary ____________________________________
  pre-commit: commands succeeded
  congratulations :)

[mnowster]

@dnephin 💖 look at those ordered imports!!! HAPPY! Interesting that it's broken up multi imports and done them as individual separate imports, I've not seen it done that way before and I'm not sure I particularly like it but I can totally forgive it for the clarity it brings in ordering.

Brilliant. LGTM 👍

[aanand]

_Nice._ This is going to break a lot of open PRs' builds, but it's got to be done at some point.

LGTM

[dnephin]

Thanks!

The import ordering I think originally comes from https://code.google.com/p/soc/wiki/PythonStyleGuide#Imports_grouping_and_order

I just realized I should probably also mention this in the contributing guide

[dopry]

1. As a contributor and developer, It would have been nice to see the import re-ordering in it's own commit, if not branch. It's not apparent reading the commit log that this change happened. Also resolving merge conflicts in re-ordering is not something kdiff or even most developers do efficiently due to a large number of small similar looking hunks.
2. If there are going to be standards for import grouping and orders it should be included in a codestyle guide so that other contributors can follow it. It's not enough to just correct a style problem, you also need to address it going forward or it will get re-introduced.
3. There is no guidance on conditional imports, which is causing Run on windows by bypassing dockerpty. #1900 PR to break now and I have no feedback on how to get my PR to pass.

[dnephin]

As a contributor and developer, It would have been nice to see the import re-ordering in it's own commit, if not branch

I'm confused, it was in it's own commit (59d4f30)

Also resolving merge conflicts in re-ordering is not something kdiff or even most developers do efficiently

It is unfortunate that this will cause a lot of conflicts, but it's a one-time cost. Going forward it should actually prevent more conflicts. The pre-commit hooks for ordering and flake8 should make it easy to resolve. You can basically just accept both versions of the conflict, then run pre-commit, and it will tell you which are duplicate, and order the rest.

If there are going to be standards for import grouping and orders it should be included in a codestyle guide so that other contributors can follow it.

Having an automated way of enforcing it makes it really easy to follow, you just run the commit hooks and it ensures you're following the standard. The documentation is covered by pep8.

I've added the docs for using these hooks in this PR #1918. Sorry they weren't included immediately. If you think they need more details, please comment on that PR.

It's not enough to just correct a style problem, you also need to address it going forward or it will get re-introduced.

Totally agree, that's what we're doing here! The automated checks run as part of the CI build.

There is no guidance on conditional imports

If you run the steps documented here #1918 it will correct the issue for you. Again, sorry the docs came after the initial PR.