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

[Bug] D417 not raised for missing parameter when docstring contains further sections #459

Open
nioncode opened this issue Mar 7, 2020 · 5 comments

Comments

@nioncode
Copy link

nioncode commented Mar 7, 2020

The following example does not raise a D417:

def foo(a, b):
    """Foobar.

    Args:
        a: test

    Raises:
        KeyError: description
    """
    pass

There is also no error raised when replacing Raises with Returns, i.e. D417 is broken for both.

Without the Raises section, everything works correctly, i.e.:

def foo(a, b):
    """Foobar.

    Args:
        a: test

    """
    pass

prints D417: Missing argument descriptions in the docstring (argument(s) b are missing descriptions in 'foo' docstring).

@samj1912
Copy link
Member

samj1912 commented May 5, 2020

Yeah - the issue is related to how we treat conventions as a bundle of error codes, instead of a first class entity. If we indeed take advantage of the fact that the user is specifying the convention type - we can avoid such ambigious cases. @Nurdok what are your thoughts on this? Also currently any google style docstring that has a returns section yields no errors for other sections since its detected as a numpy section.

@Nurdok
Copy link
Member

Nurdok commented May 28, 2020

@samj1912 - as I commented on #478, I think using the --convention flag to determine how to parse docstrings SGTM, but I'd like to see a more detailed proposal of how this would work.

@adamjstewart
Copy link

@samj1912 if I understand correctly, any docstring containing a section that is present in both Google-style and Numpy-style, such as:

  • Returns
  • Raises
  • Yields
  • See Also
  • Notes
  • References
  • Examples
  • Attributes
  • Methods

will be interpreted as Numpy-style instead of Google-style, is that correct? These are obviously very common sections for docstrings, so this effectively means that auto-detection of Google-style docstrings doesn't currently work.

There are many possible fixes for this:

  1. Instead of checking for numpy section names, check for numpy-only section names
  2. Use the --convention flag to force a particular style
  3. Add a new flag to force a particular style and keep --convention as a bundle of error codes only

Do you have a particular preference for any of these? I think 1 is the quickest/easiest and most flexible since it would work most of the time, even if a single project mixes both Numpy and Google style in different methods. I'm also perfectly okay with 2 or 3 since my projects don't mix styles and I can easily specify which convention I use.

I'm happy to submit a PR to try to implement any of these options, although I may need pointers on which files contain the style autodetection code.

@Mr-Pepe
Copy link
Contributor

Mr-Pepe commented Apr 13, 2022

@adamjstewart The file checker.py contains the following, starting from line 1062:

@check_for(Definition)
def check_docstring_sections(self, definition, docstring):
    """Check for docstring sections."""
    if not docstring:
        return

    lines = docstring.split("\n")
    if len(lines) < 2:
        return

    found_numpy = yield from self._check_numpy_sections(
        lines, definition, docstring
    )
    if not found_numpy:
        yield from self._check_google_sections(
            lines, definition, docstring
        )

The Google checks are skipped if anything is found in self._check_numpy_sections.

I think treating a docstring as a numpy docstring although I have explicitly configured pydocstyle to use the Google convention is a bug. Option 2 proposed by @adamjstewart sounds like the best way forward to me because it is less fragile then the first option and does not introduce the complexity of Option 3.

My proposal:

  • Introduce a Convention class that holds the error codes to check for
  • Pass the convention as an argument to the check function (defaults to pep257)
  • Use the convention to initialize the ConventionChecker
  • Call the appropriate methods depending on the used convention

The first three steps would be a refactoring and the fourth step would fix the actual bug. Let me know if that sounds reasonable and I would volunteer to implement this.

@RomainTT
Copy link

Hi all,
It seems this issue is not fixed yet. Any plans or implementation ideas about it ?
Thank you.

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

No branches or pull requests

6 participants