readthedocs / sphinx-autoapi

A new approach to API documentation in Sphinx.
https://sphinx-autoapi.readthedocs.io/
MIT License
415 stars 126 forks source link

Catch all exceptions when reading files #384

Closed felixvd closed 1 year ago

felixvd commented 1 year ago

Importing a file with this content:

from typing import TypedDict
MyDict = TypedDict("MyDict", {'badField': List[], 'goodField': int})

Results in an extremely unhelpful error message that does not mention the offending file:

Extension error (autoapi.extension):
Handler <function run_autoapi at 0x7fbc8d1be3a0> for event 'builder-inited' threw an exception (exception: Parsing Python code failed:
invalid syntax (<unknown>, line 2))

This change fixes the behavior.

For the record, the Exception that was not caught was of type astroid.exceptions.AstroidSyntaxError.

AWhetter commented 1 year ago

The build is failing because there's invalid syntax in the file. If we don't raise an exception, then users might assume that everything has been documented correctly, when actually the file with the offending syntax error won't have been documented. Why is catching all exceptions beneficial?

felixvd commented 1 year ago

If we don't raise an exception, then users might assume that everything has been documented correctly

I am not sure I can follow your argument. Not raising an exception is the current behavior when catching IOError, TypeError, ImportError. They all cause files not to be documented and only log a warning. If you say this is undesirable, the current code should be fixed, e.g. like this:

-        except (IOError, TypeError, ImportError):
+        except (IOError, TypeError, ImportError) as e:
            LOGGER.debug("Reason:", exc_info=True)
            LOGGER.warning(
                "Unable to read file: {0}".format(path),
                type="autoapi",
                subtype="not_readable",
            )
+           raise e
        return None

I can add the raise e line to this PR if you prefer.

Why is catching all exceptions beneficial?

  1. Any exception causes the file to be undocumented, so they should all be caught and reported.
  2. Currently, the user is not told which file needs to be fixed. That is a significant problem.