terrencepreilly / darglint

A python documentation linter which checks that the docstring description matches the definition.
MIT License
482 stars 41 forks source link

Consistency with flake8-docstrings and flake8-rst-docstrings #28

Closed sobolevn closed 5 years ago

sobolevn commented 5 years ago

There are quite popular tools to lint existing docstrings:

It would be awesome to have a note in the README if this project is compatible with them or not.

It would be also awesome to include a test of some kind if they should be compatible.

terrencepreilly commented 5 years ago

They are compatible. I'll add it to the readme. A test sounds like a good idea, but I haven't yet added an integration suite. I'll have to give some though on how I'd like to do it.

sobolevn commented 5 years ago

You can have a look at:

for some inspiration ๐Ÿ™‚

terrencepreilly commented 5 years ago

I have added a compatibility test. It runs flake8 against a local set of repositories first without darglint error reports, then with. It makes sure that the second set of reported errors is a superset of the first.

To run the tests, you would first clone some repos into darglint/integration_tests/repos/. Then you can run the integration tests with tox:

tox -e pre-commit

Or run just the compatibility tests with pytest, provided that you have flake8, flake8-docstrings, flake8-rst-docstrings, and the current version of darglint installed:

pytest integration_tests/compatibility_tests.py
sobolevn commented 5 years ago

Awesome ๐ŸŽ‰

sobolevn commented 5 years ago

@terrencepreilly it still happens to me with darglint==1.1.0

That's the code that is not compatible between flake8-rst-docstrings and darglint: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/checker.py#L112-L119

The online rst editor is on flake8-rst-docstrings side, example

So, I either get this (without the line brake between long description and the next argument):

    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.

        """

Output:

ยป flake8 ./wemake_python_styleguide/checker.py

./wemake_python_styleguide/checker.py

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

  ^

Or this happens when I add an empty line between previous long description and the next argument:

    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.

        """

Output:

ยป flake8 ./wemake_python_styleguide/checker.py

./wemake_python_styleguide/checker.py

  110:1    DAR101 Missing parameter(s) in Docstring: - file_tokens
  based on its parameters. This one is executed once per module.
  ^

  110:1    DAR101 Missing parameter(s) in Docstring: - filename
  based on its parameters. This one is executed once per module.
  ^

Am I missing something?

Thanks a lot for this tool! ๐Ÿ‘

sobolevn commented 5 years ago

@terrencepreilly btw, welcome to the family https://github.com/wemake-services/wemake-python-styleguide/blob/master/pyproject.toml#L83 ๐Ÿ˜„

You are now an official part of wemake-python-styleguide project!

terrencepreilly commented 5 years ago

So, I tried to reproduce this and was able to. However, I don't think this is because of an interaction between darglint and flake8-rst-docstrings. I made a dockerfile which copies your examples above. (You can run it with docker build -t reproduce:latest . && docker run -t -i reproduce flake8 . My output is

with_lb.py:5:1: E302 expected 2 blank lines, found 1
with_lb.py:8:1: E302 expected 2 blank lines, found 1
without_lb.py:5:1: E302 expected 2 blank lines, found 1
without_lb.py:8:1: E302 expected 2 blank lines, found 1
without_lb.py:25:1: RST203 Definition list ends without a blank line; unexpected unindent.

So, only the example without the line break raises an exception in flake8-rst-docstrings. (Although I may have just misunderstood the problem, though.)

sobolevn commented 5 years ago

I guess the reason is that it is already fixed in master, but not in 1.1.0. Try to change: RUN pip install flake8 flake8-rst-docstrings to RUN pip install flake8 flake8-rst-docstrings darglint

Here are my results:

ยป flake8 without_lb.py with_lb.py --isolated --select=RST,DAR
without_lb.py:24:1: RST203 Definition list ends without a blank line; unexpected unindent.
with_lb.py:22:1: DAR101 Missing parameter(s) in Docstring: - file_tokens
with_lb.py:22:1: DAR101 Missing parameter(s) in Docstring: - filename
terrencepreilly commented 5 years ago

I don't think there's a compatibility issue. Running flake8 with only darglint installed produces

./with_lb.py:23:1: DAR101 Missing parameter(s) in Docstring: - file_tokens
./with_lb.py:23:1: DAR101 Missing parameter(s) in Docstring: - filename

And running with only flake8-rst-docstrings produces

./without_lb.py:25:1: RST203 Definition list ends without a blank line; unexpected unindent.

So we should expect that running them together would produce all three errors, which is the case.

If you're referring to the erroneous "missing parameter(s) in Docstring" errors, those are expected because the item definitions are separated from the others by a newline. Darglint first splits a docstring into sections (based on newlines and keywords), and then runs parsers on each individual section. (See the explanation here). So, since these two separated items don't have a section marker, they look like part of the long description. I'd like to correct this at some point, but it's a difficult problem to solve correctly without the runtime exploding.

sobolevn commented 5 years ago

I am calling a "compatibility issue" that darglint and flake8-rst-docstrings ask for completely opposite code formats. That's what we observe on these cases:

# First format:
without_lb.py:24:1: RST203 Definition list ends without a blank line; unexpected unindent.

# Second format:
with_lb.py:22:1: DAR101 Missing parameter(s) in Docstring: - file_tokens
with_lb.py:22:1: DAR101 Missing parameter(s) in Docstring: - filename

darglint is not happy with with_lb.py while flake8-rst-docstrings is happy with it. And darglint is fine with without_lb.py while flake8-rst-docstrings raises a warning there.

And I completely agree that this is the cause:

Darglint first splits a docstring into sections (based on newlines and keywords), and then runs parsers on each individual section. (See the explanation here). So, since these two separated items don't have a section marker, they look like part of the long description.

terrencepreilly commented 5 years ago

I'll open a separate issue for this. I think we should be able to handle this, though it might be a little difficult to handle all the edge cases.

sobolevn commented 5 years ago

Thanks a lot ๐Ÿ’ฏ