◐ Shell
reader mode source ↗
Skip to content

bpo-35476: clear error in _imp_create_dynamic_impl#14738

Closed
isidentical wants to merge 1 commit into
python:masterfrom
isidentical:bpo35476
Closed

bpo-35476: clear error in _imp_create_dynamic_impl#14738
isidentical wants to merge 1 commit into
python:masterfrom
isidentical:bpo35476

Conversation

@isidentical

@isidentical isidentical commented Jul 13, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

clear error in _imp_create_dynamic_impl

https://bugs.python.org/issue35476

@isidentical isidentical changed the title bpo35476: clear error in _imp_create_dynamic_impl Jul 13, 2019
brettcannon
brettcannon requested changes Jul 19, 2019
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I don't think this is quite doing what @ericsnowcurrently meant when he said "the error should be cleared" as an option in the issue. Instead, I believe what he was thinking was that there be an elif (PyErr_Occurred()) clause to go after the if (mod != NULL) which then clears the exception.

@bedevere-bot

bedevere-bot commented Jul 19, 2019

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@brettcannon

brettcannon commented Jul 19, 2019

Copy link
Copy Markdown
Member

I'm going to kick off a discussion over on the issue which I think may change the approach this PR takes.

@brettcannon

brettcannon commented Jul 22, 2019

Copy link
Copy Markdown
Member

@isidentical please note the conversation on the issue is advocating for explicit exception raising, so that changes the approach you've taken here.

@brettcannon

brettcannon commented Jul 26, 2019

Copy link
Copy Markdown
Member

CLosing as @ericsnowcurrently pointed out the current semantics cover the bug.

@isidentical isidentical deleted the bpo35476 branch December 7, 2019 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon requested changes

@encukou encukou Awaiting requested review from encukou

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@warsaw warsaw Awaiting requested review from warsaw

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@isidentical @bedevere-bot @brettcannon @the-knights-who-say-ni