DISC: Validating internal docstring presence, completeness and style

[simonjayhawkins]

We currently have guidelines/checks for docstrings https://pandas.pydata.org/docs/development/contributing_docstring.html for the api facing methods/functions following the numpydoc docstring guide https://numpydoc.readthedocs.io/en/latest/format.html. since we also use sphinx for building the on-line documentation.

Many of our internal functions don't have docstrings and this could be a barrier to new contributions. For the more complex/undocumented internals, I am concerned that this may also affect the sustainability of the project if only a handful of contributors can grok the code.

The pandas docstring guidelines could be considered too heavyweight for internal functions and therefore may be factor in the incomplete coverage of docstrings for internal functions.

[simonjayhawkins]

To start the discussion, I propose that we adopt google style guide for internal docstrings/code comments. http://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings

We could also considering using pydocstyle http://www.pydocstyle.org/en/latest/ to validate docstrings against this style. (although may involve technically difficulties having two standards)

[jorisvandenbossche]

What aspect of the google style guide do you propose to adopt? Or the full section about docstring comments? If that is the case, I don't really see why we would start using a different format to document parameters for internal functions vs public functions.

[simonjayhawkins]

The driver for adopting a different format, would be primarily to make it easier to have code checks for docstring coverage/quality for all of the codebase not just the public api. Is it feasible to use the current validation on all docstrings?

The google style also includes styles for module docstrings and pydocstyle allows these to be validated.

With the introduction of type annotations, having the types in the docstring is redundant (for internal functions) and duplication can allow inconsistencies if there is no checks to keep them in-sync. In the google style for Args, "List each parameter by name. A description should follow the name, and be separated by a colon and a space." so no need to repeat types.

To enforce a style, we need validation. Feasibility on using our current validation across the codebase could either be discussed here or raise a new issue. There is #15580 but "this issue focused on public functions" #15580 (comment)

[simonjayhawkins]

I would like the outcome of this discussion to be either the current "pandas docstring guide" renamed "pandas api docstring guide" with a second, much more lightweight guide.. "pandas internal docstring guide" or rewrite the current guide to acknowledge the distinction. #32033 (comment)

[simonjayhawkins]

Or the full section about docstring comments?

adopting "section 3.8 Comments and Docstrings" in it's entirety means that "pandas internal docstring guide" is effectively already written with pydocstyle providing the validation.

[jbrockmendel]

Would it be viable to maintain a single standard and gradually expand it to more files/functions?

[simonjayhawkins]

I opened this issue primarily in order to improve coverage, so my motivation is to achieve greater breadth rather than depth. I therefore feel that having a more lightweight standard is more likely to achieve that goal.

I do see issues with having a different standard and therefore opened the discussion with the google suggestion for feedback. I see some middle ground with the single standard approach, by just adopting a subset of the current standard for the internal docstrings.

Potentially, we could then modify and use the current validation on the subset.

We could take the subset approach a bit further changing say the requirement for types where type annotations exist. However, too may exceptions and we are in danger of creating "yet another" standard. In this were to be the case, adopting an exisiting standard, say the google standard may be more preferable.

[jbrockmendel]

so my motivation is to achieve greater breadth rather than depth

I'm not sure I understand the distinction. Can you give an example?

We could take the subset approach a bit further changing say the requirement for types where type annotations exist.

This relates to a topic I've been thinking about: should we treat annotations as "this is what you should pass" or "you can safely assume that this is what is passed"? e.g. in core.indexing we do the latter

[simonjayhawkins]

I'm not sure I understand the distinction. Can you give an example?

def sanitize_array(
    data, index, dtype=None, copy: bool = False, raise_cast_failure: bool = False
):
    """
    Sanitize input data to an ndarray, copy if specified, coerce to the
    dtype if specified.
    """

vs

def is_sequence(obj) -> bool:
    """
    Check if the object is a sequence of objects.
    String types are not included as sequences here.

    Parameters
    ----------
    obj : The object to check

    Returns
    -------
    is_sequence : bool
        Whether `obj` is a sequence of objects.

    Examples
    --------
    >>> l = [1, 2, 3]
    >>>
    >>> is_sequence(l)
    True
    >>> is_sequence(iter(l))
    False
    """

I think that time/effort creating/reviewing docstings for functions that are less complex/more obvious would be better spent on more complex/less obvious functions.

A more lightweight standard would hopefully reduce the time/effort creating/reviewing docstrings.

[simonjayhawkins]

This relates to a topic I've been thinking about: should we treat annotations as "this is what you should pass" or "you can safely assume that this is what is passed"?

There's also another.. "this is what you could pass".

When I'm adding annotations, i go though the function and the inputs of the functions called to try and establish what types are acceptable.

I think there maybe some merit in being more restrictive on the type accepted, i.e. should pass.

This is not the place for that discussion though. If there is not already an issue for this, maybe we should have one.

[simonjayhawkins]

"you can safely assume that this is what is passed"

that is effectively what MonkeyType does. https://pypi.org/project/MonkeyType/

[jorisvandenbossche]

First, I am -1 on using a different (conflicting) style guide for internal docstrings than for external ones. I personally don't see the advantage of that.

I think that time/effort creating/reviewing docstings for functions that are less complex/more obvious would be better spent on more complex/less obvious functions.

That's a valid concern, but how does that relate to the styling of the docstrings? Because you have the feeling that with numpydoc style too much time goes to formatting details? And with google style this would be less?

Note that with numpydoc standard, we can also have a subset of the rules to check for internal docstrings. We don't need to check every detail of it if we don't want to.

And insn't that basically what your PR #32033 did? It checked some general formatting issues (using pydocstyle) that didn't conflict with numpydoc.

[simonjayhawkins]

That's a valid concern, but how does that relate to the styling of the docstrings? Because you have the feeling that with numpydoc style too much time goes to formatting details? And with google style this would be less?

mainly just against the formatting of args requiring types and conforming to the numpydoc type notation which is a different standard to the python typing notation used for type annotations.

Note that with numpydoc standard, we can also have a subset of the rules to check for internal docstrings. We don't need to check every detail of it if we don't want to.

That's basically the aim of this issue and what i'd like to get consensus agreement on. A more lightweight standard. i.e. not to need to meet all the requirements of numpydoc for an internal docstring. (to apply when reviewing/creating)

And insn't that basically what your PR #32033 did? It checked some general formatting issues (using pydocstyle) that didn't conflict with numpydoc.

basically, yes. using a subset of error codes in the first instance that are compatible with numpydoc.

If you recall there was a review request to update the docs 😉 Hopefully this discussion can provide the content, see #32773 (comment)

However, #32033 didn't cover the type annotation/docstring redundancy issue.