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

DOC503 false positive when same exception class is documented multiple times #171

Closed
ssbarnea opened this issue Sep 27, 2024 · 6 comments
Closed

Comments

@ssbarnea
Copy link

ssbarnea commented Sep 27, 2024

 DOC503: Method `Infrastructure.__post_init__` exceptions in the "Raises" section in the docstring do not match those in the function body Raises values in the docstring: ['ValueError', 'ValueError', 'ValueError']. Raised exceptions in the body: ['ValueError'].

And the docstring looks like

        """Initialize the infrastructure.

        Raises:
            ValueError: If the container engine is not found.
            ValueError: If the container name is not set.
            ValueError: If both only_container and include_container are set.
        """

To be honest, I do not see anything wrong with docstring and having to rewrite it to workaround the linting error might make the text much harder to read.

I suppose that removing duplicates before checking should address this issue.

@ssbarnea
Copy link
Author

ssbarnea commented Sep 27, 2024

What makes it worse is that my attempts to add # flake8: noqa: DOC503 did not work in any way. I had to use the baseline feature to ignore it.

@Amar1729
Copy link
Contributor

Amar1729 commented Sep 28, 2024

hey @ssbarnea I actually chose to write the doc503 check this way. not a bug but I am open to discussion.

when I wrote the check I couldn't find any docstring style guideline that discussed the possibility of specifying the exception multiple times (eg https://google.github.io/styleguide/pyguide.html#doc-function-raises ) - please link if you find any because I ended up having to make this opinionated choice: if an exception is raised for different reasons, I figured it would be more clear to refactor to raise different exceptions, one for each reason.

it may be a bug that flake8 noqa comment doesn't work though, I'll look into that.

@jsh9
Copy link
Owner

jsh9 commented Sep 28, 2024

What makes it worse is that my attempts to add # flake8: noqa: DOC503 did not work in any way. I had to use the baseline feature to ignore it.

Hi @ssbarnea , I think # noqa: DOC503 is sufficient, and you don't need flake8 in the comment. And please also note that you'll need to run pydoclint as a flake8 plugin in order to do in-line ignores like this. (Please see documentation for further explanations: https://jsh9.github.io/pydoclint/#24-native-vs-flake8)

@jsh9
Copy link
Owner

jsh9 commented Sep 28, 2024

I'm also leaning towards the current implementation (i.e., merging duplicated exceptions in the docstring). For example, if a certain exception is raised for different reasons, we can write the docstring like this:

    """Initialize the infrastructure.

    Raises:
        ValueError: If the container engine is not found, the container
                    name is not set, or both only_container and 
                    include_container are set.
    """

Also, Python's "raises" section in docstrings remind me a bit of Java's "throws" section in the signature:

void doSomething() throws ArithmeticException, ArrayIndexOutOfBoundsException {
    // do something
}

Each exception is also only mentioned once in the signature.

That said, if there are other style guides that is in favor of not merging exceptions, we can consider making this a configurable behavior.

@jsh9
Copy link
Owner

jsh9 commented Oct 20, 2024

Given our discussion above, I'm closing this issue. If anyone has more comments to share, please feel free to comment below. And we can reopen it (or create a separate issue) if needed.

@jsh9 jsh9 closed this as completed Oct 20, 2024
@jsh9
Copy link
Owner

jsh9 commented Jan 12, 2025

Apparently, people's opinion shifts over time, as does mine.

I'm open to adding an option to let users choose whether to deduplicate exceptions. See more details here: #193 (comment)

There will be a PR soon by @s-weigand and/or myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants