autodoc: only mock specified modules with their descendants. #4413
Conversation
rjarry
changed the title
sphinx/ext/autodoc/importer.py
Outdated
| @@ -84,13 +84,13 @@ def __getattr__(self, name): | |||
| class _MockImporter(object): | |||
| def __init__(self, names): | |||
| # type: (List[str]) -> None | |||
| self.base_packages = set() # type: Set[str] | |||
| for n in names: | |||
| self.base_packages = set() # type: Set[Tuple[str, ...]] | |||
There was a problem hiding this comment.
Is this base package? It is better to me to rename like target_packages this because the purpose is changed.
sphinx/ext/autodoc/importer.py
Outdated
| package = tuple(name.split('.')) | ||
| # check if name is (or is a descendant of) one of our base_packages | ||
| for pkg in self.base_packages: | ||
| if package[:len(pkg)] == pkg: |
There was a problem hiding this comment.
I feel comparing package[:len(pkg)] is a bit unreadable.
How about this?
# self.target_packages contains package names as string (not tuple).
for pkg in self.target_packages:
if name == pkg or name.startswith(pkg + '.'):
return self
sphinx/ext/autodoc/importer.py
Outdated
| try: | ||
| importer = _MockImporter(names) | ||
| yield | ||
| finally: | ||
| importer.disable() | ||
| if importer is not None: |
There was a problem hiding this comment.
Why did you check importer here? Did you mean instantiation of _MockImporter might be failed?
There was a problem hiding this comment.
Yes, I put the explanation in the git commit message:
There was a problem hiding this comment.
Oh, sorry. I'd overlooked it. I understand.
| 'missing_package3.missing_module3', | ||
| 'missing_package1', | ||
| 'missing_package2', | ||
| 'missing_package3', |
There was a problem hiding this comment.
We have to add existing_package.missing_module pattern.
|
In addition, we have feature freeze for 1.7 release in this weekend. |
|
I'm not sure I'll have time to work on it before this weekend. If you want to take over my draft, please do. I don't mind :) |
Do not mock the ancestors of the specified modules in autodoc_mock_imports. Only mock the modules themselves and their descendants (as specified in the docs). Fix the test configs accordingly. Signed-off-by: Robin Jarry <robin@jarry.cc>
rjarry
force-pushed
the
autodoc-mock-imports
branch
from
fe24d5c to
c24dffc
Compare
|
@tk0miya I pushed the changes you suggested. Tell me what you think :) |
we need to add this for backward compatibility. Can you work it? To do that, we have to give an option to and give an option from autodoc side: (I don't have time now, so the naming is very roughly) |
|
I'm not sure adding a new option is a good idea. In fact we are only fixing bad behavior. If some users are relying on this corner case, they can can change |
|
Absolutely, adding base package will resolve it instead option. |
|
Thanks @tk0miya! |
|
Merged. Thank you for great work! |
|
Now I added an incompatible entry for this: 0d61b25. |
Do not mock the ancestors of the specified modules in
autodoc_mock_imports. Only mock the modules themselves and their descendants (as specified in the docs). This changes behavior compared to the original mock import implementation that was done in version 1.3 where all ancestors were also mocked implicitly (may require warning in release notes?).I fixed the test configs accordingly.
Fixes #2557