Skip to content

Externalize flake8 linting, introduce additional checks #2638

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.

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 1 commit into from
Jan 6, 2017
Merged

Externalize flake8 linting, introduce additional checks #2638

merged 1 commit into from
Jan 6, 2017

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Jan 5, 2017

This change is a step towards removing runtests.py (see #1673).

The exclusion list in the flake8 configuration in setup.cfg has been updated
to enable running the linter from the root of the project by simply invoking
flake8. This enables it to leverage its own file discovery and its own
multiprocessing queue without excessive subprocessing for linting every file.
This gives a minor speed up in local test runs. Before (runtests.py -j4):

  total time in lint: 130.914682

After:

  total time in lint: 20.379915

There's an additional speedup on Travis because linting is now only performed
on Python 3.6.

More importantly, this means flake8 is now running over all files unless
explicitly excluded in setup.cfg. This will help avoiding unintentional
omissions in the future (see comments on #2637).

Note: running flake8 as a single lazy subprocess in runtests.py doesn't
sacrifice any parallelism because the linter has its own process pool.

Minimal whitespace changes were required to mypy_extensions.py but in return
flake8 will check it now exactly like it checks the rest of the mypy/*
codebase. Those are also done on #2637 but that hasn't landed yet.

Finally, flake8-bugbear and flake8-pyi were added to test requirements to make
the linter configuration consistent with typeshed. I hope the additional
checks will speed up future pull requests by automating bigger parts of the
code review. The pyi plugin enables forward reference support when linting
.pyi files. That means it's now possible to run flake8 inside the typeshed
submodule or on arbitrary .pyi files during development (which your editor
could do for you), for example on fixtures. See discussion on #2629 on checks
that are disabled and why.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I have to wring my hands some more before merging this. I have two issues:

  • in my own mypy directory I have tons of throw-away scripts -- the flake8 run now checks those, reporting lots of silly issues.
  • I'm not completely happy with invoking things from runtest that themselves manage process pools; this defeats the point of the process pool in runtests. In the past there were complaints from people about this, where runtests.py -j1 would still use multiple CPUs -- a problem when you are trying to keep some other CPU usable for interactive work you're doing on the same machine.

@@ -12,6 +12,7 @@ docs/build/
.mypy_cache/
.incremental_checker_cache.json
.cache
.python3
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't belong here, I'll remove it.

@@ -15,4 +15,5 @@ install:
- python setup.py install

script:
- python runtests.py
- python runtests.py -x lint
- if [[ $TRAVIS_PYTHON_VERSION == '3.6-dev' ]]; then flake8; fi
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should change this and line 8 to 3.6 and add 3.7-dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last time I checked 3.6 still wasn't a thing which is why I kept 3.6-dev. Will change and see what happens.

@ambv
Copy link
Contributor Author

ambv commented Jan 6, 2017

I have tons of throw-away scripts

How do you keep them from being accidentally committed? If there's a .gitignore rule that applies to them, I could reuse it in our .flake8 config. I personally have a global .gitignore rule to ignore folders called ".local" and keep my throw-away stuff there.

runtests.py -j1 would still use multiple CPUs

Right, that's an easy fix, I could pass -j to flake8, too. Will change.

This change is a step towards removing `runtests.py` (see #1673).

The exclusion list in the flake8 configuration in `setup.cfg` has been updated
to enable running the linter from the root of the project by simply invoking
`flake8`.  This enables it to leverage its own file discovery and its own
multiprocessing queue without excessive subprocessing for linting every file.
This gives a minor speed up in local test runs. Before:

  total time in lint: 130.914682

After:

  total time in lint: 20.379915

There's an additional speedup on Travis because linting is now only performed
on Python 3.6.

More importantly, this means flake8 is now running over all files unless
explicitly excluded in `setup.cfg`. This will help avoiding unintentional
omissions in the future (see comments on #2637).

Note: running `flake8` as a single lazy subprocess in `runtests.py` doesn't
sacrifice any parallelism because the linter has its own process pool.

Minimal whitespace changes were required to `mypy_extensions.py` but in return
flake8 will check it now exactly like it checks the rest of the `mypy/*`
codebase.  Those are also done on #2637 but that hasn't landed yet.

Finally, flake8-bugbear and flake8-pyi were added to test requirements to make
the linter configuration consistent with typeshed.  I hope the additional
checks will speed up future pull requests by automating bigger parts of the
code review.  The pyi plugin enables forward reference support when linting
.pyi files.  That means it's now possible to run `flake8` inside the typeshed
submodule or on arbitrary .pyi files during development (which your editor
could do for you), for example on fixtures.  See discussion on #2629 on checks
that are disabled and why.
@ambv
Copy link
Contributor Author

ambv commented Jan 6, 2017

Alright, all fixed except for the open question about your one-off scripts.

@gvanrossum gvanrossum merged commit 18aae47 into python:master Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants