[Python][Doc] Iteratively enable more numpydoc checks

[asfimport]

Asof #7732 we're going to have a numpydoc check running on pull requests. There is a single rule enabled at the moment: PR01

Additional checks we can run:

ERROR_MSGS = {
 "GL01": "Docstring text (summary) should start in the line immediately "
 "after the opening quotes (not in the same line, or leaving a "
 "blank line in between)",
 "GL02": "Closing quotes should be placed in the line after the last text "
 "in the docstring (do not close the quotes in the same line as "
 "the text, or leave a blank line between the last text and the "
 "quotes)",
 "GL03": "Double line break found; please use only one blank line to "
 "separate sections or paragraphs, and do not leave blank lines "
 "at the end of docstrings",
 "GL05": 'Tabs found at the start of line "{line_with_tabs}", please use '
 "whitespace only",
 "GL06": 'Found unknown section "{section}". Allowed sections are: '
 "{allowed_sections}",
 "GL07": "Sections are in the wrong order. Correct order is: {correct_sections}",
 "GL08": "The object does not have a docstring",
 "GL09": "Deprecation warning should precede extended summary",
 "GL10": "reST directives {directives} must be followed by two colons",
 "SS01": "No summary found (a short summary in a single line should be "
 "present at the beginning of the docstring)",
 "SS02": "Summary does not start with a capital letter",
 "SS03": "Summary does not end with a period",
 "SS04": "Summary contains heading whitespaces",
 "SS05": "Summary must start with infinitive verb, not third person "
 '(e.g. use "Generate" instead of "Generates")',
 "SS06": "Summary should fit in a single line",
 "ES01": "No extended summary found",
 "PR01": "Parameters {missing_params} not documented",
 "PR02": "Unknown parameters {unknown_params}",
 "PR03": "Wrong parameters order. Actual: {actual_params}. "
 "Documented: {documented_params}",
 "PR04": 'Parameter "{param_name}" has no type',
 "PR05": 'Parameter "{param_name}" type should not finish with "."',
 "PR06": 'Parameter "{param_name}" type should use "{right_type}" instead '
 'of "{wrong_type}"',
 "PR07": 'Parameter "{param_name}" has no description',
 "PR08": 'Parameter "{param_name}" description should start with a '
 "capital letter",
 "PR09": 'Parameter "{param_name}" description should finish with "."',
 "PR10": 'Parameter "{param_name}" requires a space before the colon '
 "separating the parameter name and type",
 "RT01": "No Returns section found",
 "RT02": "The first line of the Returns section should contain only the "
 "type, unless multiple values are being returned",
 "RT03": "Return value has no description",
 "RT04": "Return value description should start with a capital letter",
 "RT05": 'Return value description should finish with "."',
 "YD01": "No Yields section found",
 "SA01": "See Also section not found",
 "SA02": "Missing period at end of description for See Also "
 '"{reference_name}" reference',
 "SA03": "Description should be capitalized for See Also "
 '"{reference_name}" reference',
 "SA04": 'Missing description for See Also "{reference_name}" reference',
 "EX01": "No examples section found",
}

cc @AlenkaF @amol- @jorisvandenbossche

Reporter: Krisztian Szucs / @kszucs
Assignee: Bryce Mecum / @amoeba

Related issues:

• [Python][Dev] Document the process to run numpydoc checks (is related to)

PRs and other links:

• GitHub Pull Request #13983
• GitHub Pull Request #15214

_{Note: This issue was originally created as ARROW-15006. Please see the migration documentation for further details.}

[asfimport]

Bryce Mecum / @amoeba:
Is this something others still want? If so, I'd like to help get this done.

To see the impact of running all validation checks instead of just two currently being run (PR01, PR10), I ran archery numpydoc without specifying any checks which seems to run all checks. I got 1458 violations across 26 checks [1] and that's even without a complete pyarrow build. Since expanding the number of numpydocs checks that get run would cause certain CI jobs to fail, would we want to expand the list strategically or just enable all of them at once?

[1] https://gist.github.com/amoeba/8020032b94fc0aa217e69b4c0308c06c

[asfimport]

Will Jones / @wjones127:
Perhaps we should start with the style-only ones first (PR06: Parameter "accept_root_dir" type should use "bool" instead of "boolean"), and skip the "missing" warnings (GL08: The object does not have a docstring). I think the missing ones will be good to do, but will require a lot more work to get enough context to properly describe each of the objects, so possibly better as a follow up. That's should narrow things down to some mechanical changes that we can get out of the way quickly (if only there were an automatic formatter).

Also, we should double check that we have instructions for how to run these checks locally, so that developers can verify their changes before pushing them.

[asfimport]

Bryce Mecum / @amoeba:
Makes sense and seems pragmatic. I'll start with a patch just to enable PR06, fix all related issues, and add a note in the Python developer docs.

[asfimport]

Bryce Mecum / @amoeba:
I added a single check, PR03, in 13983 and fixed the corresponding violations. While I was in there and looking at adding documentation on how to run the numpydoc checks, I realized there are two slightly different ways to run numpydoc:

• archery numpydoc

• archery lint --numpydoc

  I'm going to file a separate Jira for documenting how to run the checks locally and see if we can't provide just a single way.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
Thanks for looking into this @amoeba!

In general, I think we should maybe also have a brief discussion before adding a new check (and changing our code to pass the check). The ones that ensure completeness (like ensuring that there is a docstring, that all parameters are documented, there is a return statement, the titles are correct for online rendering, etc) are really useful, but there are also a bunch of style checks that might be less valuable (some might come off as a bit nitpicky, like missing "." at the end of the summary, I don't know if we want to follow all of them strictly)

[asfimport]

Bryce Mecum / @amoeba:
A brief discussion sounds like a great idea. To accompany a discussion, I put together a Google Sheet with a summary all checks and their status with respect to being enabled on CI, passing, etc. See https://docs.google.com/spreadsheets/d/1NVzQe3MA4Mh-AqsWUmwIZu9r5gB-Lmugz7mge3stYvU. What's a good venue to discuss?

[asfimport]

Will Jones / @wjones127:
Great spreadsheet! Best place for developer discussions is the dev mailing list, since it's the most public. Add a link to this ticket and the spreadsheet, and you can add a [Python][Doc] prefix to the subject to help recipients know if it's relevant to them.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
Nice overview! I think your proposal for candidate checks to enable/update looks quite sensible, thanks for that (added a few comments).

(personally not sure this requires a mailing list discussion, as it are relatively minor changes on its own, and only about documentation)

[asfimport]

Bryce Mecum / @amoeba:
I've added five more checks (GL10, PR04, PR05, RT03, YD01) to CI in #15214 I started working on PR06 but ran into an issue with how we do docstrings for pyarrow.compute functions and how numpydoc chooses to implement PR06. PR06 checks that you don't use types like 'string' and use 'str' instead. We dynamically generate the docstrings for pyarrow.compute functions from libarrow compute which uses types like MatchSubstringOptions. This fails PR06 because string is a substring.

 
I filed an an issue and a PR to get this change so we can add PR06. After that, the remainder we decided to work on are GL08, SS01, and RT01.

 

[raulcd]

There are a couple of PRs merged for this issue. @amoeba can I mark this for 11.0.0 and close this issue?
#15214
We can open other issues as follow ups

[amoeba]

Hey @raulcd, I'd be fine with that. There's more work but this ticket turned into more of an Epic than an Issue. I think we've discussed enough here that I can open a new issue to address the remainder of the work. Please feel free to close.

[raulcd]

Thanks!