Skip to content
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

Disable flake8-bugbear errors (if installed) #2629

Merged
merged 1 commit into from
Jan 3, 2017
Merged

Disable flake8-bugbear errors (if installed) #2629

merged 1 commit into from
Jan 3, 2017

Conversation

gvanrossum
Copy link
Member

These bit me since the typeshed tests now install flake8-bugbear. We're not interested in these.

@ambv
Copy link
Contributor

ambv commented Jan 3, 2017 via email

@gvanrossum
Copy link
Member Author

Which specific ones did you find not useful?

All of them. Or rather, I don't feel like fixing them since this is mature code and they feel like nagging. I want these silenced now because typeshed has this in requirements-tests-py3.txt (and I tend to share virtualenvs between related projects).

I also generally don't like "cleanup" PRs since they obscure true authorship of the code in git blame, so I discourage you from even thinking about submitting a PR that fixes these.

@ambv
Copy link
Contributor

ambv commented Jan 3, 2017

The additional checks are mostly to aid with diff reviews. This is how bugbear got started, to help contributors avoid some typical mistakes and make their code more obvious to the reader. In this sense it's supposed to make the reviewer's job easier.

I understand your concern about git blame poisoning. The situation looks like this. There's just a few classes of warnings being raised:

1     B001 Do not use bare `except:`.
9     B006 Do not use mutable data structures for argument defaults.
13    B007 Loop control variable not used within the loop body.
2     B306 `BaseException.message` has been deprecated as of Python 2.6 and is removed in Python 3.

B001 is just a single case. It would need to get fixed regardless because there will be a built-in warning for the same problem in the next version of flake8.
B006 cases are all within stubgen and stubgenc and there's no mutation of those defaults. Good to ignore.
B007 is raised mostly when there's tuple unpacking in the for loop and we only ever use some of those elements. I agree there's little point fixing them now apart from some small efficiency wins (like: using items() instead of values(), needless enumerate() use, etc.). Good to ignore.
B306 are false positives due to TypeParseError reinstating message as a field on the exception. Good to ignore.

So, my advice here:

  • ignore all B3 errors, Mypy is Python 3-only so those warnings are not going to be useful
  • ignore B007, the currently used style would poison history
  • fix the single B001 case
  • consider fixing B006 in stubgen/stubgenc so that we can still warn against it in future diff reviews; ignore B006 if you consider this warning not useful at all

Do you think that's reasonable?

@gvanrossum gvanrossum merged commit c6bae48 into master Jan 3, 2017
@gvanrossum gvanrossum deleted the bugbear branch January 3, 2017 19:30
@gvanrossum
Copy link
Member Author

I'd be happy to review a PR that implements your proposal.

@ambv
Copy link
Contributor

ambv commented Jan 5, 2017

I'd be happy to review a PR that implements your proposal.

Done, see #2638.

gvanrossum pushed a commit that referenced this pull request Jan 6, 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:

  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.
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.

None yet

2 participants