Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
gengjiawen wants to merge 1 commit into from
Closed

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

gengjiawen wants to merge 1 commit into from

Conversation

@gengjiawen
Copy link
Member

@gengjiawengengjiawen commented Jan 25, 2019•
edited
Loading

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

cc @nodejs/python @cclauss

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jan 25, 2019
@addaleax
Copy link
Member

addaleax commented Jan 25, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20330/

@addaleax
Copy link
Member

addaleax commented Jan 27, 2019

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

@cclauss
Copy link
Contributor

cclauss commented Jan 27, 2019•
edited
Loading

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

thefourtheye
thefourtheye approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@thefourtheyethefourtheye 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 @cclauss's suggested change.

@gengjiawen
Copy link
MemberAuthor

gengjiawen commented Jan 28, 2019•
edited
Loading

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

@cclauss
Copy link
Contributor

cclauss commented Jan 28, 2019•
edited
Loading

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
Copy link
MemberAuthor

gengjiawen commented Jan 28, 2019

I see, thanks for the info.

cclauss
cclauss reviewed Jan 28, 2019
View reviewed changes
tools/test.py Outdated
Copy link
Contributor

@cclausscclaussJan 28, 2019

Choose a reason for hiding this comment

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

try: fromqueueimportQueue, Empty# Python 3exceptImportError: fromQueueimportQueue, Empty# Python 2

Copy link
Contributor

@cclausscclaussJan 28, 2019

Choose a reason for hiding this comment

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

https://docs.python.org/3/howto/pyporting.html#use-feature-detection-instead-of-version-detection

@gengjiawen
Copy link
MemberAuthor

gengjiawen commented Jan 28, 2019

I will push again when I got home :)

@refack
Copy link
Contributor

refack commented Jan 28, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20381/

@refackrefack self-assigned this Jan 28, 2019
@addaleax
Copy link
Member

addaleax commented Jan 28, 2019

Landed in 08100bf

addaleax pushed a commit that referenced this pull request Jan 28, 2019
@gengjiawen@addaleax
tools: make test.py Queue part Python 3 compatible
08100bf
Signed-off-by: gengjiawen <[email protected]> PR-URL: #25701 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@refackrefack removed their assignment Jan 28, 2019
@refackrefack added the python PRs and issues that require attention from people who are familiar with Python. label Jan 28, 2019
addaleax pushed a commit that referenced this pull request Jan 28, 2019
@gengjiawen@addaleax
tools: make test.py Queue part Python 3 compatible
7ba6650
Signed-off-by: gengjiawen <[email protected]> PR-URL: #25701 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@targostargos mentioned this pull request Jan 29, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
@gengjiawen@BethGriggs
tools: make test.py Queue part Python 3 compatible
5024457
Signed-off-by: gengjiawen <[email protected]> PR-URL: #25701 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@BethGriggsBethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@gengjiawen@BethGriggs
tools: make test.py Queue part Python 3 compatible
1350d3c
Signed-off-by: gengjiawen <[email protected]> PR-URL: #25701 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@gengjiawen@MylesBorins
tools: make test.py Queue part Python 3 compatible
c6a2be2
Signed-off-by: gengjiawen <[email protected]> PR-URL: #25701 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@beevelopbeevelop mentioned this pull request May 29, 2019
Closed
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cclausscclausscclauss approved these changes

@addaleaxaddaleaxaddaleax approved these changes

+2 more reviewers

@refackrefackrefack approved these changes

@thefourtheyethefourtheyethefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pythonPRs and issues that require attention from people who are familiar with Python.testIssues and PRs related to the tests.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gengjiawen@addaleax@cclauss@refack@thefourtheye@nodejs-github-bot