Travis CI - Updates and Additions (sorted imports) #4481
Conversation
pradyunsg
changed the title
|
I'll push a news file after the Travis job.
|
|
Awesome! The only failing job is |
|
#4481 needs to be merged since it fixes the broken docs job. |
|
So, this PR would pass it's builds after #4485 is merged and this PR is rebased on top of that. |
| env: TOXENV=py36 VENDOR=no WHEELS=yes | ||
|
|
||
| - env: TOXENV=py27 | ||
| python: 2.7 |
There was a problem hiding this comment.
It looks like there is a good bit of churn in this file for no real reason that I'm seeing? What's the rationale for changing all of these around instead of just adding TOXENV=isort?
There was a problem hiding this comment.
I did it to make the keys sorted, alphabetically. Aesthetic change, not functional.
|
Should be able to rebase this now. |
|
Awesome! Will do! |
|
/request-review @dstufft @xavfernandez @pfmoore I hope nobody minds such comments? Please tell me if you do. |
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
tests/data/src/pep518-3.0/setup.py
Outdated
| @@ -1,5 +1,5 @@ | |||
| #!/usr/bin/env python | |||
| from setuptools import setup, find_packages | |||
| from setuptools import find_packages, setup | |||
|
|
|||
There was a problem hiding this comment.
There's a lot of churn like this in the PR. I'm a strong -1 on churn like this when it's purely cosmetic (I'm sure there's some sort of style guide recommendation that names should be sorted, or some such) but it's not one we've applied historically, and there's nothing in this PR that specifically talks about applying new style rules.
If you want to apply new style rules, can the change be split out into a separate PR and debated as a deliberate change, not added as part of an unrelated change? (I assume this was the fault of the flake8 update changing the default rules? We shouldn't blindly accept changes to the defaults like this - we should probably have an explicit config that defines our rules - but as I say, that's a debate for a PR specific to style rule changes).
There was a problem hiding this comment.
I'll remove the churn and put it into (yet) another PR. :)
Regarding the style changes I'll make a comment about what they are in a moment.
a debate for a PR specific to style rule changes
Which would be this PR.
| matrix: | ||
| include: | ||
| - env: TOXENV=docs | ||
| - env: TOXENV=pep8 | ||
| - env: TOXENV=py3pep8 | ||
| - env: TOXENV=isort |
There was a problem hiding this comment.
A new Travis CI job has been added that checks that all the imports in the project are in a sorted order.
The configuration for this is stored in setup.cfg in the isort section.
| env: TOXENV=py36 VENDOR=no WHEELS=yes | ||
|
|
||
| - env: TOXENV=py27 | ||
| python: 2.7 |
There was a problem hiding this comment.
These keys have been alphabetically sorted. This is an aesthetic change, not a functional one.
| skip = | ||
| _vendor | ||
| __main__.py | ||
| multi_line_output = 5 |
There was a problem hiding this comment.
This essentially sets the kind of indentation that is used by isort when there are imports that span multiple lines.
5 is the hanging import style that pip currently uses.
| known_first_party = | ||
| pip | ||
| tests | ||
| default_section = THIRDPARTY |
There was a problem hiding this comment.
Any package that is not from pip or tests would be treated as a third-party import.
| __main__.py | ||
| multi_line_output = 5 | ||
| known_third_party = | ||
| pip._vendor |
There was a problem hiding this comment.
All packages from pip._vendor should be treated like third-party packages and should be grouped with them.
| @@ -1,3 +1,15 @@ | |||
| [isort] | |||
There was a problem hiding this comment.
isort is a tool that sorts the imports in Python files; or verifies that they are. Off it's PyPI page:
isort is a Python utility / library to sort imports alphabetically, and automatically separated into sections.
I feel this is a worthwhile addition to pip since there have been multiple occasions where I've felt the need to sort the imports for neatness but avoided doing so to prevent excessive churn to the PR.
There was a problem hiding this comment.
OK, this is the key thing. I don't know whether isort is a worthwhile addition, but it should be proposed and if agreed, added in its own PR. You've opened #4520 which seems to be the changes caused by isort, but that's not the point. What I want to see debated is whether we want imports sorted. Our style checks have never in the past flagged unsorted imports as a problem - I'm assuming there's been no policy or tool change to require that, because IMO it would be a change that needed more visibility than it's getting here.
I'm also unclear - are you simply changing things to make it possible for people to run isort manually? Or are you adding infrastructure to have it run automatically? I think it's the former, but that's ultimately pointless, as imports will simply become unsorted again, as people make changes, so this becomes an ongoing maintenance task.
My two key points here are:
- My encodings PR suddenly got merge conflicts because someone (you?) sorted the imports on the master branch. This is what I consider "unreasonable churn" - by sorting, in-progress PRs are invalidated, and that's not something we should do lightly (and certainly not in an automated manner).
- I have never been required to sort imports that I add in a PR in the past. I don't see anything to enforce sorting going forward, so what will we do? Periodically run isort over the codebase, causing this sort of churn on a regular basis? If sorting is agreed, it should be enforced by flake8, not after the fact.
Regarding "I've felt the need to sort the imports for neatness but avoided doing so to prevent excessive churn to the PR" that's exactly the sort of thing I object to. You should not be changing things outside of the purpose of your PR, so the pressure to not change "just for neatness" is correct. If you feel strongly enough, then you can raise a follow-up (manual) change to sort imports in that particular case. It's unlikely anyone will object - but it's visible (if you'd done this for compat.py I'd have seen a PR "sort imports in compat.py" go past, and I'd have known what was happening with my encoding PR.
To be clear - I don't want to start an extended style debate. I find discussing code style to be frustrating, demoralising and ultimately non-productive. If you want to propose a change to our style guide to sort imports, then I'd probably object weakly, but give up reasonably fast if no-one agreed with me. But the question should be discussed openly as a style guide change that affects all contributions to pip, and not just as a cosmetic one-off change that's introduced as part of an unrelated item.
Also, I'm not suggesting you did anything wrong. But there's an unstated point here - which probably needs explicitly stating in our contribution/developer documentation - that PRs should not contain changes unrelated to the purpose of the PR, and in particular "cosmetic" changes must always be separated into their own PR.
There was a problem hiding this comment.
What I think should happen, if you want to suggest sorted imports, is a new PR:
- Title should be "Require imports to be sorted in style rules"
- It should implement a flake8 check that rejects PRs that have unsorted imports
- Optionally, it can provide a tool that automatically sorts, but that tool sould work by default on a file-by-file basis. We shouldn't default to altering the whole codebase.
- The sorting tool should be documented somewhere.
Once that PR has been agreed and merged, we can push a follow up PR that does an initial "get everything sorted" change. Ideally, we'd scan existing PRs to ensure we don't break too many, but that's probably too hard to be worthwhile.
There was a problem hiding this comment.
@pfmoore I think you missed that there's a new Travis CI job that uses the tox "isort" command that has been added...
I guess the churn in .travis.yml was the reason. (my bad)
| @@ -1,6 +1,7 @@ | |||
| [tox] | |||
| envlist = | |||
| docs, packaging, pep8, py3pep8, py27, py33, py34, py35, py36, py37, pypy | |||
| docs, packaging, pep8, py3pep8, isort, | |||
| py27, py33, py34, py35, py36, py37, pypy | |||
There was a problem hiding this comment.
Wrapped to separate the maintainance-related jobs from the test jobs.
| @@ -25,15 +26,20 @@ commands = | |||
| python setup.py check -m -r -s | |||
|
|
|||
| [testenv:pep8] | |||
| basepython = python2.7 | |||
| deps = flake8==2.3.0 | |||
| basepython = python2 | |||
There was a problem hiding this comment.
Changed python2.7 to python2 (and python3.3 to python3) since the extra specificity does not improve anything for us, IMO.
| basepython = python2.7 | ||
| deps = flake8==2.3.0 | ||
| basepython = python2 | ||
| deps = flake8==3.3.0 |
There was a problem hiding this comment.
Upgrade the version of flake8 being used since the version being used was around 2 years old.
| commands = flake8 . | ||
|
|
||
| [testenv:isort] |
There was a problem hiding this comment.
A new tox command for running isort.
There was a problem hiding this comment.
So this isn't a testing command, but actually affects the code? That seems like an abuse of tox. Shouldn't it be an invoke command (see the tasks directory) or a standalone script?
There was a problem hiding this comment.
It's a tox command because this is a check equivalent to pep8 - a linting check/test. (due to the --check-only option)
|
@pfmoore I hope that addresses your issues with this PR. :) |
|
Ah ohkay. I don't wanna start an extended debate on style rules.
True. But it is something that has been pointed out to me on multiple occasions as a thing to do in a few PRs by @xavfernandez - which is really the motivation for this - to remove the need for that manual step.
There's an additional You'll notice that the job also prints what changes have to be made to fix/sort the imports. Further, running
There has been -
True. I don't know what the appropriate action would be. Adding it to the development documentation?
Sorry about that. The PR that caused it would be #4485... Ideally that and this PR should have been merged one after the other but that didn't happen. Whoops.
I guess the pypa-bot tagging PRs with "needs rebase or merge" label would be enough to let the PR authors something happened. And when they merge/rebase the new isort job will point out any unsorted imports they'd have.
Definitely. I think we can do that in this PR? EDIT: Changed which sentences were quoted, minor formatting and changed order of responses. |
I just went and did it for the whole codebase (in #4485 and #4520) and added automated infrastructure to ensure that later contributions also abide to having sorted imports (this PR). EDIT: Changed which sentence was quoted. |
|
To make it extra clear - I'm proposing to sort imports in the entire codebase of pip. This PR adds infrastructure support to check that every (current and future) PR abides by it. #4520 makes the changes to actually make the current codebase abide by these new style rules. Is every core developer fine with such a change? /ping @pfmoore @dstufft @xavfernandez |
pradyunsg
changed the title
|
OK, thanks for the clarification, yes I had missed that infrastructure support. Personally I'm -1 on sorting imports. It feels to me like a cost with no real benefit. As far as cost is concerned, it's another (admittedly small) bump in the way of getting a PR implemented. I don't sort imports in my own code, for me readability is more important than ordering. I will often group related imports, and if no other considerations apply, I tend to find ordering by length of identifier is more readable. So for me, it'd be something I'd end up with extra "fix the pep8 violations" commits for it. That may just be me, of course - it's just that what I find readable doesn't match with what's being proposed. The biggest cost to me is that the rule makes it impossible to group imports by purpose. We don't do that much (if at all) but in my experience it's a good practice in some situations. So not being able to do it is a loss. Of course, the key point about having a rule is to avoid subjective decisions, so the above isn't an argument against the rule in itself - it's just a statement of the cost we have to accept if we feel the rule is worthwhile. As far as benefits are concerned, I don't actually see any. The obvious candidates:
Having said all this, I don't believe this is an important issue (in my view, all discussions of style rules are mostly a waste of time - "make your code readable" is the only rule I consider worthwhile, all else is basically bureaucracy). So if the rest of the pip devs want this rule, that's OK with me. |
|
I think I'd call myself +0 or +0.5 or so. I'm not fully +1 because I ultimately don't think it matters that much, but we should either not require people to sort their imports in any particular fashion in code review, or I think we should use something that can be mechnically enforced. Trying to enforce code style via code review ends up being sporadically enforced in 100% of situations which I think is the worst of both worlds. I think the primary benefit is to try and prevent redudant imports, something like: import pip.compat
# ... Other stuff
from pip.compat import PY3
# .... other stuff
from pip.compat import PY2That being said, I think if we do this, we should probably just handle it as part of the pep8 tox run, I don't see a big benefit to having it be separate (possibly it should be renamed to |
|
FWIW, my position on this is the same as @dstufft's - a +0.5. I don't think import order is something that matters much or should be manually curated. Either it should be automated or a don't care thing, with a slight personal preference for the former. I'll merge the CI jobs when I pick up the PC. |
That's a good point and would move me to -0 if this is indeed a benefit. But the rules on how to sort aren't clear to me (see my post) so I'm not sure the 3 cases you show would actually be sorted together.
Agreed. PEP 8 itself (which we follow) is silent on the matter apart from saying we should group imports as stdlib, then third party, then internal (which AIUI isort does). As this PR stands, we seem to be choosing "sort the way isort does" as our rule, which isn't ideal as those rules aren't clear (I have enough problems with some of flake8's annoying continuation line rules, that I can never get right by any means other than trial and error). Can we at least document our standard? |
|
I agree that we should document what the expectations are if we're going to start enforcing them, probably also including a message in our default PR message that points to a link where someone could find said documentation. I guess the main question since @pradyunsg said it was @xavfernandez that asked him to rearrange imports is whether he feels strongly about import order or not. If he doesn't, then since nobody seems to feel strongly, I'd suggest we just close this and let PRs just kind of let the imports go where they think it makes sense (Assuming someone isn't doing anything crazy, like randomly interspersing imports with code... but I think that flake8 handles that already), if @xavfernandez feels strongly, then we can move forward with isort and get the rules documented. |
|
Sorry, I've been quite busy lately (and will likely stay for the coming 2-3 months :-/ ). Like @dstufft said, it helps to spot patterns in the imports but also, somewhat contradictory, having clear guidelines is also a good way to avoid bike-shedding, especially in open source projects with contributors with different backgrounds and tastes. I agree, it would need a clear documentation on the expected ordering. |
|
@pradyunsg I'm still confused, I'm afraid. #4485 was merged 12 days ago. Now we have #4520 which again sorts imports (43 files changed). Is that how many ordering violations have been introduced in the 12 days since #4485 was merged, or were different settings used for the 2 isort runs? Surely we'd expect very few remaining violations to be picked up by #4520? |
The answer is - Both. Some order was changed when resolving PR merge conflicts (I assuming) and also, I added tests as a known first party package and changed the default section to third-party for unknown packages - to make pytest, pretend, mock etc. be categorized and grouped as a third party packages (something I had missed during #4485) - which meant that those imports got rearranged. Sorry for the confusion! note-to-self: If it's decided to have an automated job that checks the import order, in that case, the imports on the master branch should be sorted once this merges. #4520 essentially does that. It would be merged into master followed by merging master into this PR - fixing remaining violations (shouldn't be any, still) - followed by merging this PR into master. That way, the build on master would never break. |
If I get the go ahead - that the import order thing is something we wanna do, I'll add a section to the docs about this... And merge the jobs like @dstufft had mentioned. |
|
@pradyunsg OK, cool. Thanks for the explanation, and sorry for nitpicking. And having the defined tox job eliminates variations like this in future. (Amusing side issue - the clock on my PC is playing up, so your comment is noted as "pradyunsg commented 22 hours from now". Give Guido his time machine back when you've finished with it :-)) |
|
So... What's the verdict on this? .-. |
|
Given the +0.5, +0.5 and -0 (or -1?) I say let's go forward with this and try it out. If it ends up being annoying we can always remove it later. So if anyone feels like it's adding undue pain, feel free to kill it or propose killing it. |
|
As the negative voter, I'm OK with going ahead with this. |
|
Okay. So, I'll go ahead and run isort on the codebase and update #4520. |
|
I went ahead and updated the There's 2 outstanding PEP-8 flags pointed out by the newer version of flake8. Should I make a separate PR fixing those or fix them here itself? |
Sort the keys alphabetically.
Add pip._vendor as a third party package Use the indentation style that the project uses Add pip and tests as a known first party packages Change default section to THIRDPARTY
If it helps, our experience with isort in mozilla/treeherder has been mostly positive, once developers got used to it. Of note:
|
|
Thanks for some valuable input @edmorley! ^.^
I didn't hit anything of this kind... Maybe it's because we're using a different indentation setting. |
lock
bot
added
the
auto-locked
Changes:
.travis.ymlfileOther PRs that need to merge before this: