Skip to content

bpo-38866: Remove asyncore from test_pyclbr.py - #17316

Merged
methane merged 2 commits into
python:masterfrom
jacksonriley:topic/28533
Nov 22, 2019
Merged

bpo-38866: Remove asyncore from test_pyclbr.py#17316
methane merged 2 commits into
python:masterfrom
jacksonriley:topic/28533

Conversation

@jacksonriley

@jacksonriley jacksonriley commented Nov 21, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

This change (trivially) removes the mention of asyncore from Lib/test/test_pyclbr.py as part of issue 28533, as it is not required for this test.

https://bugs.python.org/issue38866

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 21, 2019
@jacksonriley

jacksonriley commented Nov 21, 2019

Copy link
Copy Markdown
Contributor Author

Hi @aeros, would you mind please reviewing this PR? It was raised as part of an issue you recently commented on. Thank you! :)

aeros
aeros suggested changes Nov 21, 2019
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @jacksonriley and welcome!

Based on my understanding of test_dotted_name_not_a_package, the first part (before the dot) should be a valid package name; otherwise it's not actually performing the correct test. Changing it to any valid stdlib module name should work (the part after the dot shouldn't matter), but I think in this case the most appropriate replacement for asyncore.foo would be asyncio.foo:

Comment thread Lib/test/test_pyclbr.py Outdated
@jacksonriley

jacksonriley commented Nov 21, 2019

Copy link
Copy Markdown
Contributor Author

Hi @nanjekyejoannah, you're the core dev who commented most recently on Issue 28533, would you mind please reviewing this sub-issue? Thank you! :)

aeros
aeros approved these changes Nov 21, 2019
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making the recommended changes @jacksonriley, LGTM.

@methane
methane merged commit 138e7bb into python:master Nov 22, 2019
@jacksonriley
jacksonriley deleted the topic/28533 branch November 22, 2019 13:54
@miss-islington

miss-islington commented Nov 23, 2019

Copy link
Copy Markdown
Contributor

Thanks @jacksonriley for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Nov 23, 2019

Copy link
Copy Markdown
Contributor

Sorry, @jacksonriley and @methane, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 138e7bbb0a5ed44bdd54605e8c58c8f3d3865321 3.7

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@jacksonriley @aeros Jake Taylor
bpo-38866: Remove asyncore from test_pyclbr.py (pythonGH-17316)
5d177a6 
Co-Authored-By: Kyle Stanley <[email protected]>
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@jacksonriley @aeros @shihai1991
bpo-38866: Remove asyncore from test_pyclbr.py (pythonGH-17316)
2a322e0 
Co-Authored-By: Kyle Stanley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@aeros aeros aeros approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@methane methane

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jacksonriley @miss-islington @aeros @methane @the-knights-who-say-ni @bedevere-bot