CI: add pydocstyle to code_checks
State: closed

http://www.pydocstyle.org/en/5.0.2/error_codes.html

Labels:
name: CI
description: Continuous Integration
name: Code Style
description: Code style, linting, code_checks

Comments:
looks ok to me. @jorisvandenbossche 
How does this contrast / overlap with the validations we already do with numpydoc/ the validate_docstrings script? 
many differences so will only give the main points below. can go into more fine-grained detail if required.

pydocstyle is a static checker so is not able to check dynamically created docstrings.

validate_docstrings script is only checking public api. pydocstyle is checking internal docstrings too.

pydocstyle and validate_docstrings have some overlapping but also exclusive error codes so having both is beneficial.

pydocstyle has a flake8 plugin so can see errors highlighted in ide so a benefit for contributors.

not added here, but error code 'D417 | Missing argument descriptions in the docstring' would add a lot more than consistent style. It would ensure contributors properly document their functions/methods. A lot of work to enable this globally.


lgtm. I think we can start with this and refine over time if conflicts with numpydoc arise
> lgtm. I think we can start with this and refine over time if conflicts with numpydoc arise

Ok. we could enable straight away and see how things pan out. so will take off draft status.

i suppose if issues do arise can always disable. The error codes included here are the simpler ones and it was not a lot of effort to make the changes across the codebase to comply.

we could then look to update the style guide once happy that having on the ci is smooth.


> error code 'D417 | Missing argument descriptions in the docstring' would add a lot more than consistent style

That's something that numpydoc checks as well? 
And actually, as far as I can see, pydocstyle cannot do this for numpy-style docstrings?
>That's something that numpydoc checks as well?
>And actually, as far as I can see, pydocstyle cannot do this for numpy-style docstrings?

can you elaborate?

the checks of the parameters aren't broken down into separate codes for the parameter violations and the checks are not so rigorous.

sample cli output of D417
```
$ pydocstyle pandas/io/formats/format.py  --select=D417
pandas/io/formats/format.py:968 in public method `to_html`:
        D417: Missing argument descriptions in the docstring (argument(s) buf, encoding are missing descriptions in 'to_html' docstring)
pandas/io/formats/format.py:1111 in public function `format_array`:
        D417: Missing argument descriptions in the docstring (argument(s) decimal, digits, float_format, formatter, justify, na_rep, space, values are missing descriptions in 'format_array' docstring)
pandas/io/formats/format.py:1507 in public function `format_percentiles`:
        D417: Missing argument descriptions in the docstring (argument(s) percentiles are missing descriptions in 'format_percentiles' docstring)
(pandas-dev) 
```
@simonjayhawkins Thanks for the explanation above https://github.com/pandas-dev/pandas/pull/32033#issuecomment-586731312 So I suppose the main reason to use it in addition to numpydoc is that it checks all docstrings? 

> > And actually, as far as I can see, pydocstyle cannot do this for numpy-style docstrings?
>
> can you elaborate?

That was mentioned in one of the issues on numpydoc that talked about using pydocstyle, but it was from a while ago, so probably out of date

---

If this is going to fail CI in case of violation, we should add this to the contributing guidelines (similar to black and flake8)

Also, should we add it to pre-commit hooks? (can it run on only the changed files? Or how fast it is for the full code base?)

Maybe worth a note in the contributing guide?
I seem incapable of successfully building documentation locally on my Windows machine. closing to clear queue
@simonjayhawkins for only adding some prose, there is not necessarily a need to build the docs locally. I can certainly review on technical rst aspects.
I think the ideal (but not most straightforward approach) is to update https://dev.pandas.io/docs/development/contributing_docstring.html.

I think that we need to avoid confusion between the public API functions/methods/classes that use sphinx to generate documentation for pandas users and docstrings for all other (internal) functions/method/classes that are for the benefit of pandas contributors.

A simpler approach for now could be to add a one-liner in the contributing guide instead.

Do we have a go forward on this? I still think OK and would be fine with doc updates as a follow on since this isn't drastically changing things.

Though not a strong preference either way - just don't want this to hang in limbo if it doesn't need to
If it is going to fail CI, it should be documented in the contributing docs. It doesn't need to belong, just a short mention of it should be fine.

Also:

> Also, should we add it to pre-commit hooks? (can it run on only the changed files? Or how fast it is for the full code base?)
> If it is going to fail CI, it should be documented in the contributing docs.

#32393 was intended as  precursor to this. Do we have the contributing docs available now? if so, can you provide a link, and i'll reopen #32393.
> Also, should we add it to pre-commit hooks?

i don't have these (may require a bit of effort/knowledge to setup on Windows). so will need others to push the relevant patch for this.
> Though not a strong preference either way - just don't want this to hang in limbo if it doesn't need to

no strong preference either. see https://github.com/pandas-dev/pandas/pull/32033#issuecomment-586744504.
I think we need further discussion before we address https://github.com/pandas-dev/pandas/pull/32033#issuecomment-586727246 and then update the guidance. I don't think this PR is that vehicle.
> I think we need further discussion before we address [#32033 (comment)](https://github.com/pandas-dev/pandas/pull/32033#issuecomment-586727246) and then update the guidance. I don't think this PR is that vehicle.

opened #32773, closing this to clear the queue for now.