Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Better import error #389
Better import error #389
Changes from 12 commits
701fa90
a9fc028
352db37
1fc3dfd
e72769a
de95d6e
91e8759
6246064
7e916b2
0b1714d
e1f3207
e36655c
cbfa664
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to my other comment, why check for strings and then throw the same error that we could throw in the
try_import_compiled_module_from_path
function?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in the class init, so it's only executed when the class is instantiated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we delay throwing the
ImportError
? If we can't import the module, why not throw the error with the descriptive message here instead of a warning?Do we need this "soft fail" type behavior somewhere that I missed? Otherwise I'm in favor of just throwing the
ImportError
here with the same message and only returning a module if the import is successful. But again, let me know if I missed something that makes this necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's because this is executed as part of the init process, and we don't necessarily want to raise an error. But if they try to instantiate the class and the compiled library is missing, we throw the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that makes sense. I forgot that python tries to import anything that's involved in the init process. I'll approve this now.