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

tools: make test.py Queue part Python 3 compatible #25701

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gengjiawen
Copy link
Contributor

gengjiawen commented Jan 25, 2019

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

cc @nodejs/python @cclauss

@addaleax

This comment has been minimized.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 27, 2019

This patch failed the python linter… /cc @nodejs/python

@cclauss

This comment has been minimized.

Copy link
Contributor

cclauss commented Jan 27, 2019

Just reverse the try except and change the exception to ImportError.

@thefourtheye
Copy link
Contributor

thefourtheye left a comment

LGTM with @cclauss's suggested change.

@gengjiawen

This comment has been minimized.

Copy link
Contributor Author

gengjiawen commented Jan 28, 2019

I can change it. But the root cause is the lint bug.

@cclauss

This comment has been minimized.

Copy link
Contributor

cclauss commented Jan 28, 2019

The linter is not wrong ModuleNotFoundError does not exist in Python < 3.6 thus it is an undefined name. Please replace it with ImportError.

@gengjiawen

This comment has been minimized.

Copy link
Contributor Author

gengjiawen commented Jan 28, 2019

I see, thanks for the info.

try:
from Queue import Queue, Empty # Python 2
except ModuleNotFoundError:
from queue import Queue, Empty # Python 3

This comment has been minimized.

@cclauss

cclauss Jan 28, 2019

Contributor
try:
    from queue import Queue, Empty  # Python 3
except ImportError:
    from Queue import Queue, Empty  # Python 2
@gengjiawen

This comment has been minimized.

Copy link
Contributor Author

gengjiawen commented Jan 28, 2019

I will push again when I got home :)

tools: make test.py Queue part Python 3 compatible
Signed-off-by: gengjiawen <technicalcute@gmail.com>

@gengjiawen gengjiawen force-pushed the gengjiawen:feature/test_python3 branch from 8dc295a to d30502d Jan 28, 2019

@refack

This comment has been minimized.

@refack

refack approved these changes Jan 28, 2019

@refack refack self-assigned this Jan 28, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 28, 2019

Landed in 08100bf

@addaleax addaleax closed this Jan 28, 2019

addaleax added a commit that referenced this pull request Jan 28, 2019

tools: make test.py Queue part Python 3 compatible
Signed-off-by: gengjiawen <technicalcute@gmail.com>

PR-URL: #25701
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@refack refack removed their assignment Jan 28, 2019

@refack refack added the python label Jan 28, 2019

addaleax added a commit that referenced this pull request Jan 28, 2019

tools: make test.py Queue part Python 3 compatible
Signed-off-by: gengjiawen <technicalcute@gmail.com>

PR-URL: #25701
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment