Fix "src/pip" to respect flake8-bugbear

[deveshks]

Towards #5537 (Follow-up of #8398)

This should finish adding of flake8-bugbear to pip, apart from some ignores which can be removed once Python2 support is dropped.

[deveshks]

I think this should be ready for merged, and should close out the integration of the first flake8 plugin with pip.

Do I also need to make an issue to remove the B014 ignores once we drop python 2 support (That might also serve as a general tracker for other ignores added as we add more flake-8 plugins to pip)

[deveshks]

Hi @pradyunsg

I observed that the CI broke in the latest merge to master: https://github.com/pypa/pip/runs/744912102 because the PR #8398 which was merged, we were specifically checking for a set number of error codes:

pip/setup.cfg

Line 30 in 19e739c

src/pip/*: B007,B008,B009,B014,B305

which was not included in that PR (B010)

There might be similar situations which might happen for the other active PRs, once we enable flake8-bugbear for entire pip after we merge this.

What would be the right course of action then to avoid such situations from happening again?

One way I think of solving this is to instruct on the active PRs about this change, and ask them to merge/rebase from master after this PR gets merged, and fix potential flake8-bugbear conflicts if it's possible without making any other part of tox -e lint unhappy, e.g. mypy, in which case we can add a #noqa <rule> with a comment. (I can take point for doing it if it's not considered too intrusive)

I am open to any other suggestions on how to avoid this situation for the other open PRs in future as well, until new PRs emerge with flake8-bugbear baked in pip entirely.

[sbidoul]

@deveshks @pradyunsg this kind of annoyance can happen when merging PRs that are not up-to-date with master. GitHub has an option to require that:

but that is extremely cumbersone (requiring a rebase before any merge, waiting for CI, etc).

A option that I like and use in other projects is to use a bot for merging. The bot can then check if all conditions for merging are satisfied, including a green CI after merging in master. I don't know if there exists an off-the-shelf bot that would fit all our process requirements though.

[deveshks]

A option that I like and use in other projects is to use a bot for merging. The bot can then check if all conditions for merging are satisfied, including a green CI after merging in master. I don't know if there exists an off-the-shelf bot that would fit all our process requirements though.

I agree that having such an off-the-shelf bot would require quite a bit of research to find, which satisfies both the current merge requirements, as well as is open to future requirement additions.

A cursory search came up with https://github.com/palantir/bulldozer which on first glance looks good.

[pradyunsg]

bors is the right tool if we want to do "make sure master doesn't break" with automation IMO -- I don't think we hit this issue often enough right now to introduce a new workflow, but if folks reckon we should do this, please file a new issue and @-mention me. :)

[pradyunsg]

@sbidoul For now, I think we should merge this PR, and then fix whatever failures are left in a follow up. :)

[deveshks]

@sbidoul For now, I think we should merge this PR, and then fix whatever failures are left in a follow up. :)

I agree as well, but the failures might come up every time when an existing PR which might not have a flake8-bugbear compatible change (although the numbers of such PRs might not be really high, so we can fix issues together in a bunch of PRs, or we can ask folks to merge/rebase from master.)

For the current PR which was merged, which failed at

pip/src/pip/_internal/cli/cmdoptions.py

Line 838 in 19e739c

setattr(always_unzip, 'deprecated', True)

we can either add a #noqa, or just drop --always-unzip altogether, since it's a no-op and isn't documented anywhere as per #2429 (comment)

[pradyunsg]

That's gonna be true for any new linter we add. 🤷🏻‍♂️

I don't think we'd hit this a lot (most of the ~80 PRs we have open either need to be closed or updated), so I'm not too worried. If anyone disagrees / wants us to actively deal with this stuff properly, please file an issue for dedicated discussion on this (as I mentioned above).

[deveshks]

That's gonna be true for any new linter we add. 🤷🏻‍♂️

I don't think we'd hit this a lot (most of the ~80 PRs we have open either need to be closed or updated), so I'm not too worried. If anyone disagrees / wants us to actively deal with this stuff properly, please file an issue for dedicated discussion on this (as I mentioned above).

Yes I agree that any new linter addition will affect only a few PRs, which can be taken care of easily. I think I was being overly cautious in my earlier statements.

[deveshks]

For the PR #7908 which was merged, which failed at

pip/src/pip/_internal/cli/cmdoptions.py

Line 838 in 19e739c

setattr(always_unzip, 'deprecated', True)

we can either add a #noqa, or just drop --always-unzip altogether, since it's a no-op and isn't documented anywhere as per #2429 (comment)

Created #8408 to remove --always-unzip, which should solve this issue at the source.

[sbidoul]

This should be good to go when green.

[deveshks]

This should be good to go when green.

Thanks, It's green now 😊