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)
(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
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?
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
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
I don't really have a strong preference out of the options presented, we should probably just pick one and stick with it.
(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
Created attachment 8667909 [details] [review] 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).
Attachment #8667909 - Flags: review?(cdawson)
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. :)
Attachment #8667909 - Flags: review?(cdawson) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.