-
Notifications
You must be signed in to change notification settings - Fork 167
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
Stop using the deprecated imp module when possible #208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
==========================================
- Coverage 85.16% 84.79% -0.38%
==========================================
Files 1 1
Lines 573 572 -1
Branches 111 112 +1
==========================================
- Hits 488 485 -3
- Misses 62 63 +1
- Partials 23 24 +1
Continue to review full report at Codecov.
|
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.
Some comments on your PR.
cloudpickle/cloudpickle.py
Outdated
@@ -1146,19 +1142,30 @@ def _rehydrate_skeleton_class(skeleton_class, class_dict): | |||
return skeleton_class | |||
|
|||
|
|||
def _find_module(mod_name): | |||
def _can_find_module(mod_name): | |||
""" | |||
Iterate over each part instead of calling imp.find_module directly. |
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 comment is not accurate anymore.
cloudpickle/cloudpickle.py
Outdated
file.close() | ||
return path, description | ||
if hasattr(importlib, 'util'): | ||
spec = importlib.util.find_spec(mod_name) |
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.
Note that using this on submodules will import the parent modules.
See the Warning in this SO answer
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.
Good catch: this is embarrassing as the old method based on imp does not have such a side effect. I don't know what should be done.
tests/cloudpickle_test.py
Outdated
valid_module = imp.new_module('valid_module') | ||
_find_module('valid_module') | ||
dynamic_module = types.ModuleType('dynamic_module') # noqa | ||
assert not _can_find_module('dynamic_module') |
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.
Shouldn't we also test it for sub-modules?
return False | ||
|
||
if hasattr(module, '__spec__'): | ||
return module.__spec__ is None |
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.
Nice one!! This is indeed better
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.
The old imp
-based code is fishy to me anyway. I cannot find a valid use case where __file__
would not be defined and the imp
-based thingy would work.
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.
In particular, I cannot build a valid case that would make it possible to have perfect coverage of the _is_dynamic
function.
I don't think it was covered either before.
path = None | ||
for part in mod_name.split('.'): | ||
if path is not None: | ||
path = [path] |
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 line, in particular, was never covered by past tests.
This should fix #207.