Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

D417 - inconsistent results with Google-style (possible bug) #443

Closed
ColinKennedy opened this issue Jan 4, 2020 · 10 comments · Fixed by #448
Closed

D417 - inconsistent results with Google-style (possible bug) #443

ColinKennedy opened this issue Jan 4, 2020 · 10 comments · Fixed by #448
Labels
Bugfix Waiting for Assignee This issue has been triaged as a good idea, waiting for a volunteer to implement

Comments

@ColinKennedy
Copy link

D417 does not report for all parameters in a function docstring. Take this example file, below.

"""Something here."""

def foo(skips, verbose):
    """Do stuff.

    Args:
        skips (:attr:`.Skip`):
            Lorem ipsum dolor sit amet, consectetur adipiscing elit.
            Etiam at tellus a tellus faucibus maximus. Curabitur tellus
            mauris, semper id vehicula ac, feugiat ut tortor.
        verbose (bool):
            If True, print out as much infromation as possible.
            If False, print out concise "one-liner" information.

    """
    pass


def bar(skips, verbose):
    """Do stuff.

    Args:
        skips (:attr:`.Skip`):
            Lorem ipsum dolor sit amet, consectetur adipiscing elit.
            Etiam at tellus a tellus faucibus maximus. Curabitur tellus
            mauris, semper id vehicula ac, feugiat ut tortor.
        verbose (bool): If True, print out as much infromation as possible.
            If False, print out concise "one-liner" information.

    """
    pass

Results

When I ran pydocstyle, it only complains with

foo.py:7 in public function `foo`:                                                                                                                                                                                                                                                            
        D417: Missing argument descriptions in the docstring (argument(s) verbose are missing descriptions in 'foo' docstring)      

This strikes me as inconsistent. The verbose parameter in bar is considered valid because its description starts on the same line as the parameter. skips doesn't start on the same line of the parameter in both foo and bar but pydocstyle doesn't ever complain about skips. It only seems to care about verbose.

Shouldn't pydocstyle be complaining that skips doesn't start on the same line, too?
Personally I'd prefer pydocstyle to accept that docstrings can start on the line below a parameter instead of forcing the user to write on the same like

e.g.

This is considered valid.

    Args:
        verbose (bool): If True, print out as much infromation as possible.
            If False, print out concise "one-liner" information.

This should also be considered valid.

    Args:
        verbose (bool): 
            If True, print out as much infromation as possible.
            If False, print out concise "one-liner" information.

To me, the latter feels more "right". But to be fair though, I didn't see anything in the google style guide indicating it's okay for descriptions could start one line down. So maybe you don't agree with that suggestion.

At the very least though, I'd like pydocstyle to enforce D417 consistently. skips should also be failing, which it isn't currently.

Environment Details

Python: 3.7.3
command: pydocstyle --ignore=D213,D202,D203,D406,D407 foo.py
pydocstyle version: 5.0.1

@ColinKennedy
Copy link
Author

After further testing, it seems that functions with 2+ parameters have this issue. e.g. if there are 4 parameters and they each start on the next line down, pydocstyle reports D417 for the last 3 and excludes the first parameter.

@Nurdok
Copy link
Member

Nurdok commented Jan 10, 2020

Thanks for the detailed bug report, @ColinKennedy!

PRs welcome :)

@Nurdok Nurdok added Bugfix Waiting for Assignee This issue has been triaged as a good idea, waiting for a volunteer to implement labels Jan 10, 2020
@ColinKennedy
Copy link
Author

Thanks @Nurdok I'll give this a try over the weekend.

@samj1912
Copy link
Member

I was able to debug this. The error is happening because of the use of :attr: (the colons specifically which confuse the regex)

@samj1912
Copy link
Member

This should also be considered valid.

    Args:
        verbose (bool): 
            If True, print out as much infromation as possible.
            If False, print out concise "one-liner" information.

I don't think I agree with the fact that the above is valid. I can't see any example in the google style guide which suggest that docstrings can start from the next line. I would err on the side of caution.

@samj1912
Copy link
Member

samj1912 commented Jan 10, 2020

Fixed in #448
Now it errors on both.

@ColinKennedy
Copy link
Author

ColinKennedy commented Jan 11, 2020

I can't see any example in the google style guide

To be fair, I said the same thing in my original post.

There's already a precedent for adding docstrings to the next line under. sphinx-napoleon for example is used to add Google-style and numpy support to Sphinx. And numpy already places descriptions under the next line. So I don't see a risk associated with letting Google-style also have the same option.

Not to mention, adding "next line" support helps readability when your arguments are verbose.

def foo(something=[(8, 1.0), (-1, 11.0), (3, 4, 6, 7)], another_argument=frozenset(), more=None):
    """Do something.

    Args:
        something (iter[tuple[int, float] or tuple[int]], optional): My information.
        another_argument (set[str], optional): Lorem ipsum dolor sit amet, consectetur 
            adipiscing elit. Ut erat urna, finibus non tincidunt scelerisque, commodo eget ante. 
        more (None, optional): Aliquam vel justo convallis est sollicitudin egestas et quis arcu. 
           Nam ut mauris luctus, elementum dolor eu, placerat turpis. Ut non consequat augue, 
           sit amet dapibus arcu. Vivamus mattis dui vel mi condimentum, pellente
    """

versus

def foo(something=[(8, 1.0), (-1, 11.0), (3, 4, 6, 7)], another_argument=frozenset(), more=None):
    """Do something.

    Args:
        something (iter[tuple[int, float] or tuple[int]], optional): 
            My information.
        another_argument (set[str], optional): 
            Lorem ipsum dolor sit amet, consectetur adipiscing elit. Ut
            erat urna, finibus non tincidunt scelerisque, commodo eget ante.
        more (None, optional): 
           Aliquam vel justo convallis est sollicitudin egestas et quis
           arcu. Nam ut mauris luctus, elementum dolor eu, placerat
           turpis. Ut non consequat augue, sit amet dapibus arcu.
           Vivamus mattis dui vel mi condimentum, pellente

    """

Keeping the description separate from the variable makes skimming easier. It also makes it easier for auto-formatting tools like par do its work to keep within PEP8's 79 characters.

If needed, I can make a separate issue/pull request so we can keep this one just related to the original bug.

@samj1912
Copy link
Member

If needed, I can make a separate issue/pull request so we can keep this one just related to the original bug.

Thanks, that would be great. I think it would be better to know what's the general opinion on this before implementing it.

@samj1912
Copy link
Member

samj1912 commented Jan 12, 2020

Copying from Google style guide -

. A description should follow the name, and be separated by a colon and a space. If the description is too long to fit on a single 80-character line, use a hanging indent of 2 or 4 spaces (be consistent with the rest of the file).

http://google.github.io/styleguide/pyguide.html#383-functions-and-methods

@ColinKennedy
Copy link
Author

Oh nice, thank you for the link @samj1912 . It's good to see that Google thought of this in advance. I'll go ahead and make that separate ticket

samj1912 added a commit to samj1912/pydocstyle that referenced this issue May 5, 2020
Currently, due to the way the regex was specified, the regex
matcher was getting thrown off by types that used colons.

This change makes the regex robust to such type and fixes PyCQA#443
Nurdok pushed a commit that referenced this issue May 28, 2020
Currently, due to the way the regex was specified, the regex
matcher was getting thrown off by types that used colons.

This change makes the regex robust to such type and fixes #443
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bugfix Waiting for Assignee This issue has been triaged as a good idea, waiting for a volunteer to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants