DOC503 picks up exception facilities

[nicholasjng]

Hey! We're using pydoclint with pre-commit in one of our projects, and got some linter errors with the latest release.

Those errors have to do with us using an "exception translator" function, which takes an exception coming from an API client, translates it into a builtin Python error, and then raises the translated error. For example, an HTTP403 (forbidden) would become a PermissionError.

You can find an example here: https://github.com/aai-institute/lakefs-spec/blob/d271df45654a239bacf482338ea8febe5c029886/src/lakefs_spec/spec.py#L356-L357

pydoclint picks this up and reports it as an undocumented exception type. But, of course, we do not want to document this implementation detail, since the user is never supposed to see API errors.

Following this rationale, in the AST analysis, non-exception types (as like in our case, e.g., a function translating exceptions) should not be reported as errors. What do you think?

Error message:

    142: DOC503: Method `LakeFSFileSystem.wrapped_api_call` exceptions in the "Raises" section in the docstring do not match those in the function body Raises values in the docstring: ['OSError']. Raised exceptions in the body: ['translate_lakefs_error'].

[nicholasjng]

As I typed this, I realized that solving this might be more tricky than anticipated, since any translated exception we do document, e.g. the PermissionError example from the issue report, never shows up in the AST of the function (it is hidden inside the exception translator).

One option is inlining the translator into the AST before analysis, but that is probably tedious and error-prone. At this stage, I think there's no sound way of supporting this functionality with pydoclint.

[jsh9]

Thanks for reporting this issue!

You are correct that there is no (easy) way for pydoclint to "know" what happens under the hood of translate_lakefs_error(). pydoclint does static code analysis using AST, which is very fast and meanwhile "dumb" by design.

For pydoclint to "know" what translate_lakefs_error() returns, we'd need to introduce another Python stdlib module called inspect, which is much slower than AST.

A workaround for your use case may be like this:

            except NotFoundException:
                # fall through, retry with `ls` if it's a directory.
                pass
            except ServerException as e:
                MyError = translate_lakefs_error(e, rpath=path)
                raise MyError

And then you can add MyError into your docstring.

[nicholasjng]

Thanks for the reply. I'm not really into the idea to subsume all possible errors under an identifier in the code, though, as it seems too hacky.

Could there be an option to not err when more exceptions are listed in the docstring than are found in the AST, kind of like saying "I'm aware that those errors I documented do not appear explicitly in the code, but trust me on this"?

Oh, and could you add DOC503 to the --skip-checking-raises flag? From the name of the switch, it should also swallow DOC503 violations if specified imo.

[jsh9]

Oh, and could you add DOC503 to the --skip-checking-raises flag? From the name of the switch, it should also swallow DOC503 violations if specified imo.

Right, that's an omission on my part. I'll make a code change to address this.

Could there be an option to not err when more exceptions are listed in the docstring than are found in the AST, kind of like saying "I'm aware that those errors I documented do not appear explicitly in the code, but trust me on this"?

Maybe a "noqa" comment on this specific function would work? But note that "noqa" only works in the flake8 mode, not in the native mode. (There's very minimal overhead with the flake8 mode though.)

[Marenz]

I have a variation of this error in the new version that seems also very wrong for me:

tests/test_script.py:97:1: DOC503 Function parse_and_validate exceptions in the "Raises" section in the docstring do not match those in the function body Raises values in the docstring: ['Exception']. Raised exceptions in the body: ['e'].

[..]
    Raises:
        Exception: If data is not valid.
[..]
    except Exception as e:
        print(f"Error: {e}")
        print(f"Line: {test_data.timestamp}, {line}")
        raise e

Why would it care what the variable of the exception is called?

[Marenz]

I suspect this introduced that bug

[jsh9]

Hi @Marenz , I opened a separate issue (#175) for what you reported.

[jsh9]

Closing this issue as it's a duplicate of #175

[jsh9]

The fix is published in 0.5.11.