Skip to content

Travis CI - Updates and Additions (sorted imports) #4481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 13, 2017
Merged

Travis CI - Updates and Additions (sorted imports) #4481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f36a052
Update version of flake8
pradyunsg May 15, 2017
661e2e8
Reformat .travis.yml
pradyunsg May 15, 2017
3fd4beb
Add an isort job
pradyunsg May 15, 2017
0c8592c
Skip vendored files
pradyunsg May 15, 2017
d4f91fd
isort: Ignore using setup.cfg
pradyunsg May 15, 2017
69a30e8
Don't lock down minor version when linting
pradyunsg May 15, 2017
44be0b0
isort: Have better configuration
pradyunsg May 16, 2017
0bd4ba9
:newspaper:
pradyunsg May 15, 2017
731989a
Add a comment back
pradyunsg May 29, 2017
e663dc9
:art:
pradyunsg Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 21 additions & 27 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,42 @@
language: python


matrix:
include:
- env: TOXENV=docs
- env: TOXENV=pep8
- env: TOXENV=py3pep8
- env: TOXENV=isort
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=packaging
- python: 2.7
env: TOXENV=py27
- python: 3.3
env: TOXENV=py33
- python: 3.4
env: TOXENV=py34
- python: 3.5
env: TOXENV=py35
- python: 3.6
env: TOXENV=py36
- python: nightly
env: TOXENV=py37
- python: pypy
env: TOXENV=pypy
- python: 2.7
env: TOXENV=py27 VENDOR=no WHEELS=yes
- python: 3.6
env: TOXENV=py36 VENDOR=no WHEELS=yes

- env: TOXENV=py27
python: 2.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it to make the keys sorted, alphabetically. Aesthetic change, not functional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These keys have been alphabetically sorted. This is an aesthetic change, not a functional one.

- env: TOXENV=py33
python: 3.3
- env: TOXENV=py34
python: 3.4
- env: TOXENV=py35
python: 3.5
- env: TOXENV=py36
python: 3.6
- env: TOXENV=py37
python: nightly
- env: TOXENV=pypy
python: pypy
- env: "TOXENV=py27 VENDOR=no WHEELS=yes"
python: 2.7
- env: "TOXENV=py36 VENDOR=no WHEELS=yes"
python: 3.6
allow_failures:
- python: nightly


install: travis_retry .travis/install.sh


script: .travis/run.sh


notifications:
irc:
channels:
# This is set to a secure variable to prevent forks from notifying the
# IRC channel whenever they fail a build. This can be removed when travis
# implements https://github.com/travis-ci/travis-ci/issues/1094.
# The actual value here is: irc.freenode.org#pypa-dev
- secure: "zAlwcmrDThlRsZz7CPDGpj4ABTzf7bc/zQXYtvIuqmSj0yJMAwsO5Vx/+qdTGYBvmW/oHw2s/uUgtkZzntSQiVQToKMag2fs0d3wV5bLJQUE2Si2jnH2JOQo3JZWSo9HOqL6WYmlKGI8lH9FVTdVLgpeJmIpLy1bN4zx4/TiJjc="
use_notice: true
- secure: zAlwcmrDThlRsZz7CPDGpj4ABTzf7bc/zQXYtvIuqmSj0yJMAwsO5Vx/+qdTGYBvmW/oHw2s/uUgtkZzntSQiVQToKMag2fs0d3wV5bLJQUE2Si2jnH2JOQo3JZWSo9HOqL6WYmlKGI8lH9FVTdVLgpeJmIpLy1bN4zx4/TiJjc=
skip_join: true
use_notice: true
Empty file added news/4481.trivial
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions pip/utils/appdirs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ def _get_win_folder_with_ctypes(csidl_name):

return buf.value


if WINDOWS:
try:
import ctypes
Expand Down
1 change: 1 addition & 0 deletions pip/vcs/mercurial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,5 @@ def check_version(self, dest, rev_options):
"""Always assume the versions don't match"""
return False


vcs.register(Mercurial)
12 changes: 12 additions & 0 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
[isort]
Copy link
Member Author

@pradyunsg pradyunsg May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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).
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I think should happen, if you want to suggest sorted imports, is a new PR:

  1. Title should be "Require imports to be sorted in style rules"
  2. It should implement a flake8 check that rejects PRs that have unsorted imports
  3. 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.
  4. 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.

Copy link
Member Author

@pradyunsg pradyunsg May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

skip =
_vendor
__main__.py
multi_line_output = 5
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_third_party =
pip._vendor
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All packages from pip._vendor should be treated like third-party packages and should be grouped with them.

known_first_party =
pip
tests
default_section = THIRDPARTY
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any package that is not from pip or tests would be treated as a third-party import.


[tool:pytest]
addopts = --ignore pip/_vendor --ignore tests/tests_cache

Expand Down
16 changes: 11 additions & 5 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped to separate the maintainance-related jobs from the test jobs.


[testenv]
setenv =
Expand All @@ -25,15 +26,20 @@ commands =
python setup.py check -m -r -s

[testenv:pep8]
basepython = python2.7
deps = flake8==2.3.0
basepython = python2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed python2.7 to python2 (and python3.3 to python3) since the extra specificity does not improve anything for us, IMO.

deps = flake8==3.3.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrade the version of flake8 being used since the version being used was around 2 years old.

commands = flake8 .

[testenv:py3pep8]
basepython = python3.3
deps = flake8==2.3.0
basepython = python3
deps = flake8==3.3.0
commands = flake8 .

[testenv:isort]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new tox command for running isort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@pradyunsg pradyunsg May 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tox command because this is a check equivalent to pep8 - a linting check/test. (due to the --check-only option)

basepython = python3
deps = isort==4.2.5
commands = isort --recursive --check-only --diff pip tests

[flake8]
exclude = .tox,*.egg,build,_vendor,data
select = E,W,F