DOC: Make doc decorator a class and replace Appender by doc
State: closed

- [x] work for #31942
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Docs

Comments:
## Error
The error in commit [6e4b036](https://github.com/pandas-dev/pandas/pull/33160/commits/6e4b0365318d7308aa4cbc697fe08a2af4c20c23) was:
```
Traceback (most recent call last):
  File "<string>", line 3, in <module>
  File "/home/runner/work/pandas/pandas/pandas/__init__.py", line 54, in <module>
    from pandas.core.api import (
  File "/home/runner/work/pandas/pandas/pandas/core/api.py", line 15, in <module>
    from pandas.core.arrays import Categorical
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/__init__.py", line 8, in <module>
    from pandas.core.arrays.datetimes import DatetimeArray
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/datetimes.py", line 46, in <module>
    from pandas.core.arrays import datetimelike as dtl
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/datetimelike.py", line 51, in <module>
    from pandas.tseries import frequencies
  File "/home/runner/work/pandas/pandas/pandas/tseries/frequencies.py", line 29, in <module>
    from pandas.tseries.offsets import (
  File "/home/runner/work/pandas/pandas/pandas/tseries/offsets.py", line 1289, in <module>
    class CustomBusinessMonthEnd(_CustomBusinessMonth):
TypeError: function() argument 1 must be code, not str
Check import. No warnings, and blacklist some optional dependencies DONE
##[error]Process completed with exit code 1.
```

## Fix
We could fix this by converting doc decorator from function to class. 

The change was made in commit [21fa045](https://github.com/pandas-dev/pandas/pull/33160/commits/21fa0456f2a8cb1b1effc97e54edad225ed8d938).
Do you mind providing some more details of what are you doing here? Looks reasonable, but not sure I understand what's the problem, and how this change fixes it.
> Do you mind providing some more details of what are you doing here? Looks reasonable, but not sure I understand what's the problem, and how this change fixes it.

Hi @datapythonista. I didn't investigate this error very thoroughly, but my guess will be this:

Our original decorator returns a function, not a class. We will get this kind of error when we are using `doc` to decorate a class and then have another class take the decorated class as a base class. The following example will give the same `TypeError`:
```python
@doc("This is a sample docstring")
class A:
    pass

class B(A):
    pass
```

The fix here is to convert function decorator to class/callable decorator.
I see, didn't think of that. I don't think you need to inherit from the class to be a problem, even if you don't inherit I think our decorator is transforming it to a function and won't work.

But I see two different changes here:
- Applying the doc changes to `func` instead of creating a `wrapper`
- Implementing the decorator as a class, and not as a function

You're implementing both, but I think they are independent, aren't they? I think if we remove `wrapper` and change `func`, even if the decorator is not a class, that would still solve the problem.

I'm happy with the changes, but just to make sure I'm understanding this correctly. Thanks!
I agree with you that the `wrapper` function causes this problem, but I also think implementing the decorator as a class is the way we solve this problem. I guess I misunderstand what you are trying to say here.

For my understanding, if we have to use a function, we would have something like:
```python
def decorator(func):
    @wraps(func)
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs)
    wrapper.__doc__ = "This is a sample docstring"
    return wrapper
```

That is the fewer function to be as a decorator I can think of. I am not sure if we can remove either `decorator` or `wrapper` here. In my mind, the simplest way to avoid `wrapper` here is to move `decorator` to a class and implement the `__call__` method.
Decorators are a bit tricky. If you want to do something like:
```python
@decorator
def foo():
   pass
```
which is equivalent to:
```python
def foo():
   pass
foo = decorator(foo)
```
Then, we could simply use:
```python
def decorator(func):
    func.__doc__ = "This is a sample docstring"
    return func
```
Since we call the decorator, then we actually have:
```python
@decorator("PARAM")
def foo():
   pass
```
Which is equivalent to:
```python
def foo():
   pass
foo = decorator("PARAM")(foo)
```
And then you need the `wrapper`, the function that will be returned by `decorator("PARAM")`.

We currently have `doc`, `decorator` and `wrapper`. And `wrapper` is the final function that will be returned and will replace the original decorated function. If we then decorate a class, the class will be replaced by the function `wrapper`, and everything will break. My point was that instead of doing that, we can simply modify the function or class received as a parameter `func`, and that way things will work, whether we decorate a class, or a function.
> We currently have `doc`, `decorator` and `wrapper`. And `wrapper` is the final function that will be returned and will replace the original decorated function. If we then decorate a class, the class will be replaced by the function `wrapper`, and everything will break. My point was that instead of doing that, we can simply modify the function or class received as a parameter `func`, and that way things will work, whether we decorate a class, or a function.

@datapythonista Yes, you are right. I didn't think about that. Also, I agree that the use function looks better here. 

One more thing wants to confirm: `args` & `kwargs` vs `appenders` & `substitution`, which one makes more sense to you? I am tangled in the names and trying to find evidence to convince myself one is better than the other. If other names make more sense, please also point out. 
@HH-MWB this PR is removing some of the typevars to ensure that decorated functions/classes have their typing signatures preserved. I recall discussing this before. do we have tests?
> @HH-MWB this PR is removing some of the typevars to ensure that decorated functions/classes have their typing signatures preserved. I recall discussing this before. do we have tests?

Hi @simonjayhawkins. Yes, I would be happy to add some tests and/or switch back to original type, but I guess I am a little bit confusing here. 

I was changing type annotation from `F` to `Callable` to matches the name changes here, and just simply assuming `F` is the same as `Callable`. 

Would you mind provide some more information about `F`? Does it also represent classes? What's the difference between it and `Callable`?


@simonjayhawkins can you have a look at @HH-MWB question?

@HH-MWB, once you know about the typing, can you address the latest comments, merge master, and leave the CI green, so we can merge this please? Thanks!
> Would you mind provide some more information about `F`? Does it also represent classes? What's the difference between it and `Callable`?

see https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators for an explaination on `F` and why it's necessary.
> Not sure about the names, may be I'd use:
> 
> * `supplements` -> `docstrings`
> * `substitutes` -> `params`
> * `call` -> `decorated` or `func_or_class`

I think this names makes sense to me, especially use `decorated` to replace `call`. I would like to make those changes first, and we can always change them if we have future discussion and get a better name.


@datapythonista 

> Not sure about the names, may be I'd use:

naming is hard.

> If we finally use the decorator, probably you can rename the params `args` and `kwargs` to the names you're giving in the class.

i don't think it makes sense to change the names if not using a class

> I checked it again, and I have a small preference for having the decorator as a function (it'll be slightly simpler), but happy to move forward either way, up to you.

looking again at the https://github.com/pandas-dev/pandas/pull/33160/commits/21fa0456f2a8cb1b1effc97e54edad225ed8d938, i think the class approach may be clearer is some of the logic had been moved from \_\_call__ to \_\_init__.

The \_\_call__ would then be simpler, maybe look similar to the current Appender/Substitution, and the naming maybe be more natural.

We also have an underlying bug to fix. i.e. remove the extra wrapper returning a function.

So my preference would be to do just the fix (maybe raise a issue for clarity and posterity) here. i.e. no renaming.








I personally think implementing the docorator with a function instead of a class is simpler. But just my opinion, happy with any of them.

For the naming, I think the generic `args` and `kwargs` are the best choice when you don't really know what you expect to receive. As an example, if `plot(kind, *args, **kwargs)` will just call a matplotlib function with whatever you're passing them, I think the names are the best. But in the other hand, when the parameters have a very specific meaning, like in this case, I think actual names are better. If I see a signature `foo(*args, **kwargs)`, I'd expect that `foo('pandas')` and `foo(name='pandas')` are somehow equivalent. Which in this case it's completely wrong, because `args` and `kwargs` are totally different concepts. So in this case, I think it's much more appropriate to use descriptive names.

Of course not a big deal, I'm ok calling them `args` and `kwargs`. But I do see a drawback.

@HH-MWB CI is red, if you don't mind having a look. Thanks!
> Of course not a big deal, I'm ok calling them `args` and `kwargs`. But I do see a drawback.

OK I do see your reasoning and feel free to merge. (As a follow-up, I would like to see this decorator implemented as a class at some point. I think the naming may then become more natural)

It looks like this is changing the stacklevel at which warnings are being issued, not obvious to me why
> It looks like this is changing the stacklevel at which warnings are being issued, not obvious to me why

I guess the reason is this PR removed function `wrapper` which used to wraps decorated function. 

The original decorator looks like
```python
def doc(*args, **kwargs):
    def decorator(func):
        @wraps(func)
        def wrapper(*args, **kwargs):
            return func(*args, **kwargs)
        wrapper.__doc__ = "some docstring"
        return wrapper
    return decorator
```

The modified decorator will be
```python
def doc(*args, **kwargs):
    def decorator(func):
        func.__doc__ = "some docstring"
        return func
    return decorator
```

Hi,

I replaced `Appender` with `doc` in a number of places in another PR. I found that the current implementation does not play well with the dynamic method creation. As illustrated in this [PR](https://travis-ci.org/github/pandas-dev/pandas/jobs/676629879), the `test_apply_with_string_funcs` test fails. [Here](https://github.com/pandas-dev/pandas/pull/33633/files#diff-03b380f521c43cf003207b0711bac67fR11153) are the changes I made. The issue is that the `doc()` decorator for some reason changes the signature of the `DataFrame.std()` method.

It would be great if you considered this use of `doc()` in your reimplementation and made sure that it works for the dynamic method creation.

If it helps here is more detail about the situation:

- the `std()` method is created dynamically [here](https://github.com/pandas-dev/pandas/blob/master/pandas/core/generic.py#L10089)
- when using the current `doc()` implementation, the [test](https://github.com/pandas-dev/pandas/blob/b630cdbc33c6e766cc1bfec857b44b04e9db08f8/pandas/tests/frame/test_apply.py#L163) fails for the [testcase](https://github.com/pandas-dev/pandas/blob/b630cdbc33c6e766cc1bfec857b44b04e9db08f8/pandas/tests/frame/test_apply.py#L158) where axis 1 is selected. It fails because in this case `std()` is calculated along the wrong axis. This happens because the `apply()` method does not [pass](https://github.com/pandas-dev/pandas/blob/b630cdbc33c6e766cc1bfec857b44b04e9db08f8/pandas/tests/frame/test_apply.py#L164) this argument correctly.
- the axis argument is not passed correctly because when the test runs for `std()` and the test case where `axis=1`, [this line](https://github.com/pandas-dev/pandas/blob/b630cdbc33c6e766cc1bfec857b44b04e9db08f8/pandas/core/apply.py#L162) is not executed.
- the control does not enter the `if` branch because `sig` from [this line](https://github.com/pandas-dev/pandas/blob/b630cdbc33c6e766cc1bfec857b44b04e9db08f8/pandas/core/apply.py#L160) has no arguments or keyword arguments.
- this does not happen when `Appender()` is used. In that case `sig` has the correct arguments and key word arguments as expected for `std()` including `axis` and `axis=1` is coeerctly passed down by the `apply()` machinery.

Hope this helps with improving the `doc()` decorator.

> > It looks like this is changing the stacklevel at which warnings are being issued, not obvious to me why
> 
> I guess the reason is this PR removed function wrapper which used to wraps decorated function.
> 

Yes, you are right. I had the same issue in my PR. You need to decrement [this value](https://github.com/pandas-dev/pandas/pull/33021/files#diff-34bddc5b2c962f58d24f594d80a8b520R787) by 2 to make the tests pass after removing the wrapper function.
Thanks @smartvinnetou for providing this information. I guess this new implementation would fix the stacklevel issue. 
> Just to make sure I understand the stachlevel issue. Some functions were before wrapped more than once with `Appender` and `Substitution` and now they are just wrapped once, and that's changing the numbers of levels the warning is stacked. Is that correct?

Hi @datapythonista. I am not 100% sure about this,but my guess is: the old `doc` decorator has 2 extra stacklevel than `Appender` and `Substitution`, but the new `doc` decorator has the same stacklevel.

Our old decorator has that extra wrapper function which increased the stacklevel by 2. @smartvinnetou has a merged PR using the old decorator, and the stacklevel has been increased by 2 to pass the CI/CD checks. Now we have to switch it back.


> Do you mind generating the warning that you're changing in `core/arrays/datetimelike.py` and add the output in a comment in this PR, so we can have a look, and make sure we're not breaking anything with this PR please?

The error message might be this:
```
Traceback (most recent call last):
  File "<string>", line 3, in <module>
  File "/home/runner/work/pandas/pandas/pandas/__init__.py", line 54, in <module>
    from pandas.core.api import (
  File "/home/runner/work/pandas/pandas/pandas/core/api.py", line 15, in <module>
    from pandas.core.arrays import Categorical
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/__init__.py", line 8, in <module>
    from pandas.core.arrays.datetimes import DatetimeArray
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/datetimes.py", line 46, in <module>
    from pandas.core.arrays import datetimelike as dtl
  File "/home/runner/work/pandas/pandas/pandas/core/arrays/datetimelike.py", line 51, in <module>
    from pandas.tseries import frequencies
  File "/home/runner/work/pandas/pandas/pandas/tseries/frequencies.py", line 29, in <module>
    from pandas.tseries.offsets import (
  File "/home/runner/work/pandas/pandas/pandas/tseries/offsets.py", line 1289, in <module>
    class CustomBusinessMonthEnd(_CustomBusinessMonth):
TypeError: function() argument 1 must be code, not str
Check import. No warnings, and blacklist some optional dependencies DONE
##[error]Process completed with exit code 1.
```

We can also goto the failed commit [6e4b036](https://github.com/pandas-dev/pandas/pull/33160/commits/6e4b0365318d7308aa4cbc697fe08a2af4c20c23) for more detail information.
Bump @datapythonista @simonjayhawkins 
Hello @HH-MWB! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







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

##### Comment last updated at 2020-05-21 01:44:13 UTC
Thanks @HH-MWB for sticking with this. can you merge upstream/master one more time and ping on green
Thanks @HH-MWB