1192957 - Add isort to the Travis run to prevent regressing Python import style

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P4)

Description

[Ed Morley [:emorley]]

Bug 1192661 cleaned up the Python imports.
We could run `isort --check-only` as part of the Travis run to avoid this regressing.

I've filed a few upstream issues which would likely need to be fixed first:
https://github.com/timothycrosley/isort/issues/298
https://github.com/timothycrosley/isort/issues/296
https://github.com/timothycrosley/isort/issues/297

Plus we'll also need to:
* Check we can't get flake8 to do this for us
* See what the impact is on the travis run time (running it inside my Vagrant VM takes 45s, but that's likely due to Virtualbox shared folders being slow - and I can't run it outside the VM since the python packages have to be installed for isort to be able to figure out whether a reference is first or third party)

Comment 1

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #0)
> https://github.com/timothycrosley/isort/issues/298

--check-only also checks for formatting, the docs were wrong (now updated). We'll just have to deal with this.

> https://github.com/timothycrosley/isort/issues/296

--skip uses non-obvious syntax, which has now been documented.

> https://github.com/timothycrosley/isort/issues/297

No reply yet, but we can work around it using |-p tests|.

> * Check we can't get flake8 to do this for us

There isn't a pep8 error code we can turn on that's relevant:
http://pep8.readthedocs.org/en/latest/intro.html#error-codes

flake8's list of extensions also doesn't cover one that does this:
http://flake8.readthedocs.org/en/latest/extensions.html#existing-extensions

Comment 2

[Ed Morley [:emorley]]

(Ignore; testing a BMO bug)

Comment 3

[Ed Morley [:emorley]]

So there's no easy way to get isort to not worry about how the lines are wrapped, so we need to pick what wrapping style it's going to enforce.

The best looking options are:

* Force multi-line single-column output if there is more than one class being imported from a module, eg:
from treeherder.webapp.api import (artifact,
                                   bug,
                                   job_log_url,

* Use single-column output as above, but only use it if the line would be too long without wrapping, eg allow:
from treeherder.webapp.api import artifact, bug, job_log_url
...but if the line were longer than N characters, fall back to single column.

* Use a grid based wrapping style, eg:
from treeherder.webapp.api import (artifact, bug, job_log_url, jobs, logslice,
                                   note, performance_data, refdata)

The advantages of forcing multi-line if more than one class, would be that it's much easier to predict what will pass vs fail, since you don't have to remember line line cutoffs. The downside is greater verbosity, for the simpler 2-3 class imports which would have been happy on a single line.

Will, do you have any thoughts?

Comment 4

[Ed Morley [:emorley]]

s/line line/line length/

Also for the full list of wrapping styles isort supports, see:
https://github.com/timothycrosley/isort#multi-line-output-modes

Comment 5

[Ed Morley [:emorley]]

I've also filed two more issues against isort:

"Support running `isort` with no additional options on the CLI"
https://github.com/timothycrosley/isort/issues/338

"isort is really slow when using --recursive since no way to efficiently exclude directories"
https://github.com/timothycrosley/isort/issues/337

Comment 6

[William Lachance (:wlach)]

I don't really have a strong preference out of the options presented, we should probably just pick one and stick with it.

Comment 7

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #5)
> I've also filed two more issues against isort:

Both of these have been fixed in the release the author specially created for them :-)

(In reply to William Lachance (:wlach) from comment #6)
> I don't really have a strong preference out of the options presented, we
> should probably just pick one and stick with it.

wfm - thank you

Comment 8

[Ed Morley [:emorley]]

Attachment: Add isort to the Travis run

(The diff is actually much smaller than it looks, most of it is auto-generated by running isort; see individual commits for clarity).

Comment 9

[Cameron Dawson [:camd]]

Comment on attachment 8667909 [details] [review]
Add isort to the Travis run

This is awesome!  I admit, I didn't go through the last commit where you applied the changes to our imports.  I hope that's ok.  Everything else looks good.  You'll have to rebase, of course.

If you'd like me to look at the code changes in detail, please n-i me and I'll do so.  Otherwise, if tests pass, I trust the tool.  :)

Comment 10

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/8be78f1f57cc2487371a7f9a72c3aa0e944825c9
Bug 1192957 - Add isort v4.2.2 to dev.txt

https://github.com/timothycrosley/isort

https://github.com/mozilla/treeherder/commit/1e599b6681b890b3d5a97a2aaf6869b6dc047fb5
Bug 1192957 - Add config for isort

It can now be run with just `isort` and will do the right thing.

The chosen multi-class import style is:

from third_party import (lib1,
                         lib2,
                         lib3
                         ...)

See the docs added in a later commit, or:
https://github.com/timothycrosley/isort/blob/develop/README.rst
https://github.com/timothycrosley/isort/wiki/isort-Settings

https://github.com/mozilla/treeherder/commit/40c90e33658b2ebc65df3da6733f6974e90c951b
Bug 1192957 - Docs: Explain the Python import style and how to run isort

In the future, the new "Code Style" page will also be used to explain
other styles/linters used in the project, eg JavaScript / ESlint.

https://github.com/mozilla/treeherder/commit/571c57af7163fbda08fd6453a6556a09a61525a0
Bug 1192957 - Mass-update python import style using isort

To fix pre-existing deviations from the style that will be enforced on
Travis in the next commit.

https://github.com/mozilla/treeherder/commit/c554745a2c8d1893c4dbb4d3462f895cd691f7b7
Bug 1192957 - Add isort to the Travis run

Any import style deviations will cause the Travis run to fail, with the
diff output to the Travis log.

Comment 11

[Ed Morley [:emorley]]

This is now documented at:
https://treeherder.readthedocs.org/common_tasks.html#running-the-tests
https://treeherder.readthedocs.org/code_style.html#python-imports