PyCQA / pydocstyle

docstring style checker
http://pydocstyle.org
MIT License
1.11k stars 189 forks source link

False negatives for D402 #284

Open ns-cweber opened 7 years ago

ns-cweber commented 7 years ago

The following function fails the D402 check with version 0.7.0:

def subclasses(cls):
    """Retrieve all subclasses (direct and transitive) from `cls`.

    :param cls: The class to search.
    :type cls: ``type``

    :returns: A generator of subclasses.
    :rtype: ``iter`` of ``type``
    """
    for subclass in cls.__subclasses__():
        for subsubclass in subclasses(subclass):
            yield subsubclass
        yield subclass

Presumably the D402 rule just sees the method name followed by an lparen?

shacharoo commented 7 years ago

Indeed it does. This would also happen on later versions. A quick workaround would be to change the first line to Retrieve all direct and transitive subclasses fromcls`` while we somehow solve this 😄

@Nurdok , perhaps we should compare the parameters in the method's declaration to the ones in the docstring? Or somehow try to validate that what appears in the parentheses are not parameter names?

Nurdok commented 7 years ago

I think that validating that the content inside the parentheses equals the parameter names might be a good fix.

shacharoo commented 7 years ago

That might be problematic @Nurdok. What if you copied the signature to the docstring but had a typo? or changed the number of parameters and forgot to change the docstring? There will be a lot of false-negatives 😕.

ns-cweber commented 7 years ago

@FarmerSez Seems like you could get pretty close by matching on a valid Python function signature (function name, lparens, comma-delineated-python-exprs, rparens) and comparing the function names. This would weed out a lot of false negatives and also prevent the false positive you described without throwing a false negative for a function name followed by text in parens.

shacharoo commented 7 years ago

@ns-cweber that's true except if the parentheses contain only one word: finds all subclasses (directly) using ...., but I'm okay with that.

chaoflow commented 5 years ago

Another one to take into consideration is:

def stat(path):
    """Wrap os.stat() for ease of mocking."""  # noqa: D402
    return os.stat(path)

I think we should match for valid function names only, not prefixed by a dot, directly followed by a opening parenthesis. This should also handle the original request on this issue.

seebi commented 3 years ago

Just another false negative example we should avoid:

@click.group(cls=CmemcGroup)
def scheduler():
    """List, inspect, enable/disable or open scheduler(s).

    Schedulers execute workflows in specified intervals. They are identified
    with a SCHEDULERID. To get a list of existing schedulers, execute the
    list command or use tab-completion.
    """

removing "(s)" solved the issue.