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

tests: Handle ImportError explicitly, improve comparisons against None #14903

Merged
merged 4 commits into from Dec 13, 2018

Conversation

Projects
None yet
6 participants
@daniel-s-ingram
Copy link
Contributor

commented Dec 9, 2018

No description provided.

daniel-s-ingram added some commits Dec 9, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

meh ACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

ACK 1b89074

I think we should use is None/is not None instead of == None/!= None throughout the code base. The former is safer and faster. Feel free to open such a PR :-)

git grep -E '(!=|==) *None' -- "*.py" is your friend.

@daniel-s-ingram

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Will do - thanks, @practicalswift!

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

@daniel-s-ingram Great! When doing that don't forget to enable flake8 check E711 in test/lint/lint-python.sh to make sure your fix becomes permanent until the end of time :-)

And don't forget the technical rationale in the PR description: otherwise people who aren't familiar with how Python handles is vs == will incorrectly claim your fix is cosmetic :-)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

utACK c9ba253

@laanwj laanwj changed the title Handle exception as ImportError tests: Handle ImportError explicitly Dec 13, 2018

@laanwj laanwj changed the title tests: Handle ImportError explicitly tests: Handle ImportError explicitly, improve comparisons against None Dec 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

I think we should use is None/is not None instead of == None/!= None throughout the code base. The former is safer and faster. Feel free to open such a PR :-)

Agree. This is overall an improvement, IMO. I did change the PR title to be more comprehensible and in line with the actual changes.

utACK c9ba253

@laanwj laanwj merged commit c9ba253 into bitcoin:master Dec 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 13, 2018

Merge #14903: tests: Handle ImportError explicitly, improve compariso…
…ns against None

c9ba253 Add E711 to flake8 check (Daniel Ingram)
17b5520 Compare to None with is/is not (Daniel Ingram)
1b89074 Change '== None' to 'is None' (Daniel Ingram)
16d2937 Handle exception as ImportError (Daniel Ingram)

Pull request description:

Tree-SHA512: aa5875ea3d9ac47ac898545ff023b511042cd377ea0c4594074daae119f3d4f3fc295721aad94a732a907086ecb32bce19a8eed38adf479f12663c4e8944f721
@@ -25,7 +25,7 @@
import codecs
try:
from urllib.request import Request,urlopen
except:
except ImportError:

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 13, 2018

Member

This exception handling is a vestige from when github-merge.py supported Python 2 and Python 3. We only support Python 3 now so we should be able to remove it entirely and just import from urllib.request.

MarcoFalke added a commit that referenced this pull request Dec 13, 2018

Merge #14947: scripts: Remove Python 2 import workarounds
4de11a3 Remove Python 2 import workarounds (practicalswift)

Pull request description:

  Remove Python 2 import workarounds.

  As noted by @jnewbery in #14903 (comment):

  > This exception handling is a vestige from when github-merge.py supported Python 2 and Python 3. We only support Python 3 now so we should be able to remove it entirely and just import from urllib.request.

Tree-SHA512: e0d21e6299dd62fb669ad95cbd3d19f7c803195fd336621aac72fd10ddc7431d90443831072a2e1eb2fc880d1d88eb7c3e2ead3da59f545f6db07d349af98fb3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.