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.
|
tomMoral
left a comment
There was a problem hiding this comment.
Some comments on your PR.
cloudpickle/cloudpickle.py
Outdated
| 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.
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.
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.
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.
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.
Nice one!! This is indeed better
There was a problem hiding this comment.
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.
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.
This line, in particular, was never covered by past tests.
This should fix #207.