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

autosummary: catch all exceptions when importing modules #4212

Merged
merged 1 commit into from Nov 11, 2017

Conversation

pv
Copy link
Contributor

@pv pv commented Nov 1, 2017

Subject: autosummary: catch all exceptions when importing modules

Feature or Bugfix

  • Bugfix

Purpose

Similarly to autodoc, autosummary/autolink try to import modules.
Module import may, in addition to ImportError, raise any exception,
including SystemExit. These need to be caught, since failing module
import should not cause a documentation build to fail.

Tests updated with examples that failed previously.

autodoc already does similar catching of exceptions.

Call __import__(modname), convert exceptions to ImportError
"""
try:
return __import__(modname)
Copy link
Member

Choose a reason for hiding this comment

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

We also have similar process on sphinx.ext.autodoc. There are additional process for handling warnings. It would be nice if integrated.

            with warnings.catch_warnings():
                warnings.filterwarnings("ignore", category=ImportWarning)
                with logging.skip_warningiserror(not self.env.config.autodoc_warningiserror):
                    __import__(self.modname)

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this, please do it in sphinx.ext.autodoc or a separate utils module: we don't want sphinx.ext.autodoc depending on sphinx.ext.autosummary

@@ -502,6 +503,20 @@ def import_by_name(name, prefixes=[None]):
raise ImportError('no module named %s' % ' or '.join(tried))


def _import_or_importerror(modname):
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming this to import_module? I think the function name should represent its purpose, not behavior.

@tk0miya tk0miya added this to the 1.7 milestone Nov 2, 2017
Module imports may raise any exceptions, including SystemExit, which
need to be caught.
@pv
Copy link
Contributor Author

pv commented Nov 2, 2017

Changed function name + added catch_warnings, but not the logging suppression (which uses autodoc settings). As it is now, the autosummary import handling remains separate from autodoc. In principle it could be nice to combine them, but that is more of a bigger refactoring than bugfix.

tk0miya added a commit that referenced this pull request Nov 11, 2017
@tk0miya tk0miya merged commit d5bea6b into sphinx-doc:master Nov 11, 2017
@tk0miya
Copy link
Member

tk0miya commented Nov 11, 2017

Manually merged (see #4232). Thank you for contribution!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants