BUG: Partial fix for docstring validation for parameters

[ChiefMilesEdgeworth]

This PR addresses #20298, does not close.

• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

This changes the method used to find arguments from inspect.getfullargspec to inspect.signature and changes the handling accordingly. It also now checks for "*args, **kwargs" and other variations as a doc parameter, and splits them up so they can be checked against.

[WillAyd]

There are tests for the doctoring validator - if making changes there also will need a test case(s)

[ChiefMilesEdgeworth]

Thank you! I'm working out the bugs introduced right now.

[ChiefMilesEdgeworth]

@WillAyd Would you like me to write tests for decorated functions? That was the main point addressed by this PR. If so, just let me know, and I can do that before this is merged.

Also, I'm having trouble figuring out why I'm not passing the automated linting check. Do you have any ideas?

[ChiefMilesEdgeworth]

Never mind, I found it. Sorry for all the really little commits, I just recently started working with pandas, so I'm getting used to all the small things that need to be done first.

[pep8speaks]

Hello @Nabel0721! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-13 19:10:49 UTC

[ChiefMilesEdgeworth]

I added tests for decorators. Look over it and see if there are any more you'd like me to add. If I have an idea of what to make I should be able to do it no problem, I'm just not sure what exactly would need to be added. The changes I made to the validation script were just to account for oversights, not to add functionality, so there weren't any new features to test.

[datapythonista]

Thanks for working on this.

There are two unrelated changes here, one changing how we obtain the paramters from the docstring, and one from the signature. Do you mind addressing them in separate PRs? The one of the docstrings is quite simple I think, but I don't quite understand the other one. And in case we change our mind and we want to revert, we won't want to revert both.

Regarding inspect.signature, can you point me to an example where this change is fixing a validation error. I'm sure you know what you're doing, but would like to understand what's going on, and I fail to see if we're fixing bugs, or just using a nicer API.

Thanks!

[ChiefMilesEdgeworth]

So, here's a code example to show what the difference is between getfullargspec and signature.

from functools import wraps
from inspect import getfullargspec, signature


def decorator(func):
    @wraps(func)
    def wrapper(*args, **kwargs):
        """
        This is hidden documentation that we don't want
        """
        print("The wrapper is doing stuff!")
        return func(*args, **kwargs)
    return wrapper

@decorator
def add(a, b, *, absolute=False):
    """
    This method adds a and b, with the absolute value optionally taken
    """
    return abs(a+b) if absolute else a+b

    
print(f"Using getfullargspec: {getfullargspec(add)}")
print(f"Using signature {signature(add)}")

The following script prints out:

Using getfullargspec: FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
Using signature (a, b, *, absolute=False)

getfullargspec only finds the signature of the decorator, since it doesn't search any deeper. However, signature checks if the object has the __wrapped__ attribute (which is assigned in the by the @wraps decorator to the decorated function), and if it does it checks the signature of that object instead. Using signature allows us to properly get the signature of decorated functions, which is important since we have decorators everywhere. For instance, there is a decorator for deprecating a specific kwarg. That was causing trouble before, because it overrode the signature of the function below it. This would solve that issue.

[ChiefMilesEdgeworth]

I'll put together a separate PR for the splitting on ", ". You're right that these two issues are pretty distinctly separate, and should be kept that way.

[datapythonista]

Thanks a lot for the detail explanation on signature vs getfullargsspec. Is there any case in pandas where this is an issue right now, and that it's fixed with this PR?

We've got _to_original_callable which I think it's taking care of this. I guess it could be simplified with this change, but it'd be nice to know how this change is affecting the reported docstrings.

[ChiefMilesEdgeworth]

Most of the PR02 errors we're getting from pandas.Series.str are the result of this. There are other places too, I remember finding them as I was looking around at PR02 errors. They usually pop up where the PR02 error says variables aren't defined that are in the signature of the function. If it's decorated, then it gets *args, **kwargs instead of whatever is supposed to be there.

We could run the check on the object returned by to_original_callable, but if we do that we should just remove that function and use inspect.unwrap instead. It does the same thing but covers more cases than we currently do. I was thinking of submitting that idea later anyways, since it cuts out a whole function and would result in better functionality (catching more cases).

[datapythonista]

Ah, I see. Do you think it makes sense to add one of the Series.str methods to the unit tests? Also, it'd probably make sense to generate the errors json with and without these changes, and find the differences, and see if the result is the expected one.

[ChiefMilesEdgeworth]

Yeah, I did that when I first made the changes, but I can send two files with the differences.

[ChiefMilesEdgeworth]

One file is taken from current master, so there'll be some changes that were fixed by other PRs. The main ones to look at are under Series.str.
decorator_fix.txt
master.txt

A lot of the errors are gone after applying the fix.

[datapythonista]

Sounds great. Let's split the two different things, and merge them then.

[datapythonista]

looks good, just couple of last comments

[datapythonista]

just one last comment, to make things simpler, and we're good to go

[datapythonista]

lgtm, thanks @Nabel0721, nice improvement

[WillAyd]

Read through history but not entirely clear - what does this end up improving upon?

[ChiefMilesEdgeworth]

Whenever we use a decorator on a function that follows normal decorator conventions (wrapping the original function, passing in the arguments and returning the result with some added functionality) we were losing the signature of the original function. The result was that the script would look for *args, **kwargs or whatever the signature was of the wrapper instead of the actual function's signature, and that would be used in the comparison to the docstring parameters. This resolves that by using a more up-to-date function from the builtin library inspect. It checks if a function has the __wrapped__ attribute, and if so calls itself recursively on the function in that attribute. So, with this change we'll see most of the errors due to decorators go away, although a few of the decorators need a bit of work since they don't use the @wraps decorator on their wrapper function (which sets the __wrapped__ attribute along with a few other things). If you need me to go into more specifics on any part of this, just let me know. At this point I've got a really good grasp on how decorators work and what the inspect.signature does.

[WillAyd]

Makes sense. I think generally we are inconsistent in how we maintain signatures, annotations, etc... with wrapped functions, so would welcome general ideas you have for that

[datapythonista]

We were doing it manually until now with _to_original_callable. @Nabel0721 I guess you'll see if it can be fully removed now in a follow up, right?

[ChiefMilesEdgeworth]

Yeah, honestly we could just replace the entire function with a call to inspect.unwrap

[datapythonista]

Great, please tag me when you open the PR. Thanks!

[ChiefMilesEdgeworth]

Ideally, we should only be working with signatures and wrapped functions using the builtin tools from the inspect library whenever possible. I'm fairly certain that this is what numpy uses as well, so it will create consistency with the imported docstring stuff from them.

[datapythonista]

I didn't see that Python had that unwapping functionality, that's why I implemented _to_original_callable. If we can get the actual function just calling the Python sdlib, that would be great, less code to maintain.