DOC: add guide on shared docstrings
State: closed

closes https://github.com/pandas-dev/pandas/issues/16446

xref https://github.com/pandas-dev/pandas/issues/19932

cc @jorisvandenbossche 

The main change in best practices here is the recommendation is that `Appender` is only for appending. `Substitution` should be used for substituting values.

See the changes to `fillna` that this policy requires. Personally, I find the new way clearer than the old way.

- We write the base template normally
- I like `Appender(NDFrame.method.__doc__)` better than `Appender(_shared_docs['name'] % kwargs)`
- I like `Substitution(**kwargs)` better than `%` formatting.

The downsides is that we don't have the template in a dictionary anymore and the docstring in `NDFrame` has to be the template, it can't have its variables substituted. I think that's OK.

Labels:
name: Docs

Comments:
Oh, I put this in contributing, but it should go in the docs file once that's ready.
Thanks!

> The downsides is that we don't have the template in a dictionary anymore and the docstring in NDFrame has to be the template, it can't have its variables substituted. 

Ah, yes, that's something I didn't think about. But, as long as the templates are used in classes that are never exposed to users (like NDFrame and IndexOpsMixin), then this is not a problem. But should check that we also don't use the system in actual user classes (eg I think base Index also has shared docstrings that are substituted in the index subclasses)
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=h1) Report
> Merging [#20016](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/7fd3b855e3e5ac821b4fce99085a72e042c04950?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20016/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #20016      +/-   ##
==========================================
- Coverage   91.85%   91.84%   -0.01%     
==========================================
  Files         152      152              
  Lines       49249    49236      -13     
==========================================
- Hits        45237    45223      -14     
- Misses       4012     4013       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <100%> (-0.01%)` | :arrow_down: |
| #single | `41.88% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `97.29% <100%> (ø)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.85% <100%> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.85% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.18% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `83.33% <0%> (-0.82%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `84.52% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.29% <0%> (-0.07%)` | :arrow_down: |
| [pandas/core/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYmFzZS5weQ==) | `96.78% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/dtypes/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL21pc3NpbmcucHk=) | `91.07% <0%> (ø)` | :arrow_up: |
| ... and [4 more](https://codecov.io/gh/pandas-dev/pandas/pull/20016/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=footer). Last update [7fd3b85...8c221ea](https://codecov.io/gh/pandas-dev/pandas/pull/20016?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@TomAugspurger I made a PR doing such a clean-up for Index (as this is one where we don't have always a non-user exposed base class where it is no problem to have non-substituted values): #20022 

@jreback you are OK with this approach? (see also discussion in #19932 and other PR #20022)
this is just the guide which is codifying current practice which is good

maybe add something about actually putting functions in Series/Frame that render the doc strings (while the actual doc string itself is in generic.py)
> this is just the guide which is codifying current practice which is good

no, it is not exactly codifying the current practice, but a slight variation of it (without `_shared_docs` dict).

There is one example in this PR (see for fillna in the diff, or #20022 for a bigger example)
my only comment is actually on your other PR #20022. Should clarify *when* should be using _shared_docs, I agree in Index subclasses we should prob try not to use it as much as its cumbersome.  Though it *is* nice to have the correct return class rather than Index as the generic return value. Its more informative.
> my only comment is actually on your other PR #20022. 

Can you then comment there?

> Should clarify when should be using _shared_docs, 

The proposal is to never use it, at least if there is a base class that can hold the template docstrings (like NDFrame). For classes like Index (where the base class is already user exposed), we might discuss further. But I show in the PR #20022 that at least currently we don't actually do this much (4 times for all Index methods). 
@jreback for NDFrame, we certainly still use the substitution with shared_kwargs, so are you ok with the approach in this PR for that part of the codebase? (use `NDFrame.method.__doc__` instead of `_shared_docs['method']`)
oh I c now what you did. yes I much prefer that, its way more explict. 
I know that this approach is based on the current one, and may be it's too ambitious to define a brand-new one, but given that we'll be working in most of the docstrings, I think it's worth suggesting.

Correct me if I'm wrong, but if the general case it's to reuse the docstring of the parent class as a template, why doesn't the decorator get it for us by default? Also, if `_shared_doc_kwargs` is defined as standard, why don't we render the parent docstring with the parameters defined on it?

Then, I'm sure you're already use to it, but I think naming the decorator `Appender` is not very descriptive.

So, with the next example:

    class Series(NDFrame):
        @reuse_parent_docstring
        def fillna(self, ...):
            ...

I think the decorator should be able to automatically do the right thing. And for special cases, the next syntax could also be supported:

    class Series(NDFrame):
        @reuse_parent_docstring(fillna_template, klass='Series',...)
        def fillna(self, ...):
            ...

or

    class Series(NDFrame):
        @reuse_parent_docstring(NotTheParent.fillna.__doc__, **_shared_doc_kwargs)
        def fillna(self, ...):
            ...

Also, may be it's a good time to use the new formatting syntax `'{klass}'` instead of `'%(klass)s'`?

I think this adds a bit of complexity to the decorator, but the code mode clear, so I think it's worth. Am I missing something?

> Then, I'm sure you're already use to it, but I think naming the decorator Appender is not very descriptive.

Agreed here, I think "AppendDocstring" or "AddDocstring" (or lower case) would already be more explicit (it is used for more than inheriting docstrings, so "reuse_parent_docstring" might also be confusing in certain cases).

> [about @reuse_parent_docstring] I think the decorator should be able to automatically do the right thing. 

I have thought about this as well, but the problem is that there are many parent classes, for example for Series it are in order: IndexOpsMixin, NDFrame, PandasObject, etc. So already in this case, it would not work out of the box for Series, as the majority of the methods needs to inherit from NDFrame, and not IndexOpsMixin. For DatetimeIndex there are even 5 parent classes before getting to `Index`. 
We could of course define a private attribute that holds this default parent class to inherit from.

> Also, may be it's a good time to use the new formatting syntax '{klass}' instead of '%(klass)s'?

The problem is that this does not work with strings where you also want to use { itself (for example for parameter type description listing the possibilities). It would also be annoying to have to do ``{{'optionA', 'optionB'}}``.  
Although of course it is a trade-off, so it is a choice we have to make, and we can also choose the other way.

A few things:

1. I'm OK with having the contributor explicitly write the exact method that's the template. In my mind, if you're writing a shared docstring, you'll need to be modify both the parent template and the child, so writing `@Appender(NDFrame.method)` is a good thing.
2. @jreback mentioned a clear policy for when to use the two styles. Let's call them dict-style and method-style.

*Use method-style when the parent class, where the template is written, is not public*. This include NDFrame and I think much of IndexOpsMixin. I haven't looked at the groupby ones.


3. I don't have a preference for new vs. old-style format strings.

If people are ok with that policy I'll add it to this PR.
> *Use method-style when the parent class, where the template is written, is not public.* This include NDFrame and I think much of IndexOpsMixin. I haven't looked at the groupby ones.

The groupby ones mostly just use variables holding a template (without storing it in a dict), which I think is good. And IndexOpsMixin can all use the method-style (as this is not public). 
The only remaining case that uses the dict-style is Index itself, for which I propose to not use substitution, but only inhertiting, so that way we can also use method-style (https://github.com/pandas-dev/pandas/pull/20022). 

Moved to the docstring guide now that it's in master.
@jorisvandenbossche @datapythonista `contribution_docstring.rst` isn't in any toctree. Is that deliberate?
Not sure, it is linked too from contributing.rst part about documentation. 

I am fine with adding it to the main index.rst, it's already a disaster anyhow :-) 
But ideally we would have a more nested contributing section of the docs.
I linked `contributing_docstring.rst` from `contributing.rst`. Personally I think `contributing.rst` is long enough to be splited. If that sounds good, we can probably leave it out of any toctree until then. I'm happy to take a looks at it when I find some time.
Yes I’m happy with it being split.

Sphinx warns if there’s source files that aren’t in a toctree. But it’ll be a bit before our doc build is warning free.

________________________________
From: Marc Garcia <notifications@github.com>
Sent: Monday, March 12, 2018 5:34:59 PM
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] Shared doc guide (#20016)


I linked contributing_docstring.rst from contributing.rst. Personally I think contributing.rst is long enough to be splited. If that sounds good, we can probably leave it out of any toctree until then. I'm happy to take a looks at it when I find some time.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/20016#issuecomment-372485303>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIpabu5aMRchc4nXOpMTCjQQgDa4Iks5tdvgTgaJpZM4SfCTh>.

Anything else you'd like to see here?
@TomAugspurger can you update with master?
Done.
Thanks!