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

Possible false positive for RST203 #17

Closed
sobolevn opened this issue Aug 11, 2019 · 13 comments
Closed

Possible false positive for RST203 #17

sobolevn opened this issue Aug 11, 2019 · 13 comments

Comments

@sobolevn
Copy link

sobolevn commented Aug 11, 2019

I this code that follows google-styleguide: https://google.github.io/styleguide/pyguide.html#doc-function-args

class Checker(object):
    def __init__(
        self,
        tree: ast.AST,
        file_tokens: Sequence[tokenize.TokenInfo],
        filename: str = constants.STDIN,
    ) -> None:
        """
        Creates new checker instance.

        These parameter names should not be changed.
        ``flake8`` has special API that passes concrete parameters to
        the plugins that ask for them.

        ``flake8`` also decides how to execute this plugin
        based on its parameters. This one is executed once per module.

        Arguments:
            tree: ``ast`` parsed by ``flake8``. Differs from ``ast.parse``
                since it is mutated by multiple ``flake8`` plugins.
                Why mutated? Since it is really expensive
                to copy all ``ast`` information in terms of memory.
            file_tokens: ``tokenize.tokenize`` parsed file tokens.
            filename: module file name, might be empty if piping is used.

        """

But, I have this violation raised:

» flake8 ./wemake_python_styleguide/checker.py

./wemake_python_styleguide/checker.py

  110:1    RST203 Definition list ends without a blank line; unexpected unindent.
  
  ^

But, this is considered valid:

        Arguments:
            tree: ``ast`` parsed by ``flake8``. Differs from ``ast.parse``
                since it is mutated by multiple ``flake8`` plugins.
                Why mutated? Since it is really expensive
                to copy all ``ast`` information in terms of memory.

            file_tokens: ``tokenize.tokenize`` parsed file tokens.
            filename: module file name, might be empty if piping is used.

I cannot used this, because it breaks https://github.com/terrencepreilly/darglint
I am not sure what is correct in this case.

@sobolevn sobolevn changed the title False positive for RST203 Possible false positive for RST203 Aug 11, 2019
@peterjc
Copy link
Owner

peterjc commented Aug 12, 2019

It is a pain when different linters give contradictory results.

I'm not familiar with https://github.com/terrencepreilly/darglint but it looks like pydocstrings is starting to get similar capabilities via the conventions setting (covering pep256, numpydoc or google style in various levels of completeness). This setting will be available in the next release of the flake8-docstrings plugin, see https://gitlab.com/pycqa/flake8-docstrings/merge_requests/20

What are you using the docstrings with - Sphinx apidoc maybe? Does that give any warning messages one way or the other? That is probably going to be your main concern.

This uses docutils internally, but http://rst.ninjs.org agrees with my plugin and wants the blank line otherwise it throws Definition list ends without a blank line; unexpected unindent. - so if there is a bug here, it is with docutils not my plugin.

@peterjc
Copy link
Owner

peterjc commented Aug 12, 2019

Cross reference terrencepreilly/darglint#28

@peterjc
Copy link
Owner

peterjc commented Jan 12, 2020

Cross reference terrencepreilly/darglint#56

@cjolowicz
Copy link

The underlying issue appears to be that flake8-rst-docstrings sees docstrings without any preprocessing by napoleon. Google-style docstrings are not valid reStructuredText, so this cannot possibly work. In particular, multi-line parameter descriptions will break, because an RST processor will interpret them as a definition list (which they are not). When the multi-line parameter is the last one, docutils will instead complain about unexpected indentation.

So it seems to me that this issue is in fact unrelated to Darglint, and the behavior of flake8-rst-docstrings is by design: it expects valid RST. The only way to handle this properly would be to provide a configuration option to enable preprocessing with napoleon, but this would add a dependency on Sphinx. Also note that autodoc does not perform static analysis, it imports the package. The object to which the docstring is attached gets passed to napoleon. There may be ways to make this work, but it does look like a hairy problem 😉

@peterjc
Copy link
Owner

peterjc commented Aug 6, 2020

Thanks you @cjolowicz for that analysis. I hadn't appreciated that Google-style docstrings are not expected to be valid reStructuredText - so in a sense this is means my plugin (flake8-rst-docstrings) is working perfectly.

Cross referencing #18, would you say that numpy docstrings are also not expected to be valid reStructuredText (or perhaps valid apart from the use of *arg and **kwargs)?

@cjolowicz
Copy link

The NumPy docstring standard states that it uses a subset of reStructuredText. IIUC napoleon is not used here to translate a non-RST format to RST, only to enrich the markup in an existing RST document. Looking at the various markup conventions, nothing there strikes me as invalid RST. Escaping asterisks at the start of documented names is probably a good idea, though.

@cjolowicz
Copy link

cjolowicz commented Aug 6, 2020

I had a quick look at how one might preprocess Google docstrings in your plugin. Not sure if it's a good idea, but it could look something like this:

from sphinx.ext.napoleon.docstring import GoogleDocstring

def preprocess_google_docstring(text: str, definition: Definition) -> str:
    mapping = [
        (Module, "module"),
        (Class, "class"),
        (Method, "method"),
        (Function, "function"),
    ]

    for kind, what in mapping:
        if isinstance(definition, kind):
            break

    docstring = GoogleDocstring(text, what=what, name=definition.name)
    return str(docstring)

This function would be called immediately before invoking rst_lint.lint. It would need to be hidden behind a configuration option.

You could depend on sphinx via an extra, so only projects using Google docstrings need to install it into the environment they run Flake8 from. One thing the above implementation does not take into account are configuration options for napoleon, and there are probably more rough edges and missing cases.

@peterjc
Copy link
Owner

peterjc commented Aug 6, 2020

That is useful, thank you. You're right that Sphinx / Napoleon settings could be a minefield, probably best avoided.

I think we should document that due to pre-processing, Google docstrings are not necessarily valid RST - and specifically suggesting ignoring RST203 "Definition list ends without a blank line; unexpected unindent." as a false positive in this context.

@cjolowicz
Copy link

@peterjc Apparently RST301 "Unexpected indentation" can also be triggered. This happens when the multi-line description is the last function parameter in the Google docstring, see cjolowicz/cookiecutter-hypermodern-python#497.

peterjc added a commit that referenced this issue Aug 11, 2020
@peterjc
Copy link
Owner

peterjc commented Aug 14, 2020

@sobolevn how do you feel about me closing this a "won't fix"?

Would you be more positive about adding Sphinx as an optional dependency in order to pre-process the Google docstrings? That would also be one way to solve #18.

@cjolowicz
Copy link

cjolowicz commented Sep 28, 2020

Hello @peterjc ,

Thanks for documenting this in your newest release!

I just stumbled onto another "false positive" for Google-style docstrings. There's also the case of a multi-line parameter description surrounded by other parameters:

def add(a: int, b: int, c: int) -> int:
    """Add three numbers.

    Args:
        a: An argument.
        b: An argument, which has a description spanning multiple
            lines.
        c: An argument.
    Returns:
        The sum.
    """
    return a + b + c

This results in the following warnings:

example.py:7:1: RST301 Unexpected indentation.
example.py:8:1: RST201 Block quote ends without a blank line; unexpected unindent.

Maybe we should advise Google docstring users to ignore RST201 as well? It's emitted for the line following the multi-line description.

You can reproduce this with the following script:

#!/bin/bash

virtualenv venv
. venv/bin/activate
pip install flake8-rst-docstrings
flake8 --select RST *.py

@peterjc
Copy link
Owner

peterjc commented Sep 28, 2020

I think the continuation of the "b" argument should be one space less indented, but yes. I think it does make sense to change the README example for Google style to:

[flake8]
extend-ignore =
    # Google Python style is not RST until after processed by Napoleon
    # See https://github.com/peterjc/flake8-rst-docstrings/issues/17
    RST201,RST203,RST301,

Although personally if I would consider this:

def add(a: int, b: int, c: int) -> int:
    """Add three numbers.

    Args:
        a: An argument.
        b: An argument, which has a description spanning multiple
           lines.
        c: An argument.
    Returns:
        The sum.

    """  # noqa: RST201
    return a + b + c

@peterjc
Copy link
Owner

peterjc commented Sep 28, 2020

Thank you!

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