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

[mypyc] Raise ImportError instead of AttributeError when from-import fails #10641

Merged
merged 7 commits into from
Jun 16, 2021

Conversation

97littleleaf11
Copy link
Collaborator

Description

Fixes mypyc/mypyc#707

Test Plan

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This looks good, thank you! Could you fix the ops test that fail (passing --update-data to pytest will automatically update them) and add a run test that tests the behavior, and we should be able to merge it.

@97littleleaf11
Copy link
Collaborator Author

@msullivan Thanks for your review! I will fix those tests later.

}

// The following code is a simplification of cpython/import.c/PyImport_GetModule()
Py_INCREF(module);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This incref and the decref below seem redundant, since the caller will continue to hold a reference to these.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now!

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 16, 2021

Actually, this may be missing a test. You should validate that you get ImportError on missing attribute.

@JukkaL JukkaL merged commit a9fda34 into python:master Jun 16, 2021
@97littleleaf11 97littleleaf11 deleted the fix-from-import branch August 5, 2021 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed from import raises AttributeError instead of ImportError
3 participants