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

fix: replace unsafe six.PY3 with PY2 for better future compatibility with Python 4 #10081

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jan 9, 2020

Fixes #10158.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • [trivial] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
    (edit by @plamut: opened the issue for better traceability)
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • [n/a] Appropriate docs were updated (if necessary)

We don't yet know if 3.10 or 4.0 will follow Python 3.9, but whichever it is, it will probably happen in 2020 when Python 3.9 reaches beta and work begins on Python 3.9+1.

There's some code which uses six.PY3:

if six.PY3:
    print("Python 3+ code")
else:
    print "Python 2 code"

Where:

PY3 = sys.version_info[0] == 3

When run on Python 4, this will run the Python 2 code!

Instead, use six.PY2.

Found using https://github.com/asottile/flake8-2020.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 9, 2020
@hugovk hugovk force-pushed the fix-flake8-2020 branch 2 times, most recently from 8f15925 to 4b7b089 Compare January 9, 2020 09:34
plamut
plamut previously approved these changes Jan 9, 2020
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

Testing for "is (not) PY2" instead of "is (not) PY3" is indeed more robust, thanks for catching that!

@plamut plamut dismissed their stale review January 9, 2020 09:52

The change is done to an auto-generated file

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

(speaking for the PubSub library)

The change is good, but needs to be made in the synth file that is used to generate the file changed here.

The changes to the files in other libraries are fine, as those files are not auto-generated (AFAIK).

pubsub/google/cloud/pubsub_v1/gapic/publisher_client.py Outdated Show resolved Hide resolved
@plamut plamut changed the title Fix for Python 4: replace unsafe six.PY3 with PY2 fix: replace unsafe six.PY3 with PY2 for better future compatibility with Python 4 Jan 9, 2020
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM with one final remark.

storage/tests/unit/test__signing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @hugovk!

Will delay merging for a day or so if anybody else wants to review it.

@plamut plamut added api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jan 9, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2020
@plamut
Copy link
Contributor

plamut commented Jan 10, 2020

The test failures are not related, also happen on master. Investigating.

Update: At least for logging, the test failure is caused by an out-of-date assertion - an optional timeout was added to core recently. Will be fixed in #10087.

Update 2: The Storage failures also seem to be a regression, will submit a fix for that, too (edit: submitted).

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 15, 2020
@plamut plamut merged commit 7a64502 into googleapis:master Jan 15, 2020
@hugovk hugovk deleted the fix-flake8-2020 branch January 15, 2020 15:22
This was referenced Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test for "if not PY2" in compatibility code, rather than "if PY3"
5 participants