Re: [PATCH v4 04/46] qapi: modify docstrings to be sphinx-compatible

[John Snow]

On 9/30/20 4:47 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > I did not say "sphinx beautiful", just "sphinx compatible". They will
> > not throw errors when parsed and interpreted as ReST.
>
> "Bang on the keyboard until Sphinx doesn't throw errors anymore" might
> be good enough for a certain kind of mathematician, but a constructive
> solution needs a bit more direction.  Is there a specification to
> follow?  Other useful resources?

I don't know if you are asking this question rhetorically, or in good faith.

Let me preface this by saying: This series, and these 119 patches, are 
not about finding a style guide for our docstring utilization or about 
proposing one. It is also not about rigorously adding such documentation 
or about finding ways to meaningfully publish it with e.g. Sphinx, or 
the styling of such pages.

Why bother to add docstrings at all, then? Because I needed them for my 
own sake when learning this code and I felt it would be a waste to just 
delete them, and I am of sound mind and able body and believe that some 
documentation was better than none. They are useful even just as plaintext.

Having said that, let's explore the actual style I tend to use.

I mentioned before in response to a review comment that there isn't 
really any standard for docstrings. There are a few competing "styles", 
but none of which are able to be programmatically checked and validated.

The primary guide for docstrings is PEP 257, of which I follow some but 
not all of the recommendations.

https://www.python.org/dev/peps/pep-0257/

In general,

- Always use triple-double quotes (unenforced)
- Modules, classes, and functions should have docstrings (pylint)
- No empty lines before or after the docstring (unenforced)
- Multi-line docstrings should take the form (unenforced):

"""
single-line summary of function.

Additional prose if needed describing the function.

:param x: Input such that blah blah.
:raises y: When input ``x`` is unsuitable because blah blah.
:returns: A value that blah blah.
"""

PEP257 suggests a form where the single-line summary appears on the same 
line as the opening triple quotes. I don't like this, and prefer 
symmetry. PEP257 *also* suggests that writing it my way is equivalent to 
their way, because any docstring processor should trim the first line. I 
take this as tacit admission that my way is acceptable and has merit.

What about the syntax or markup inside the docstring itself? there is 
*absolutely no standard*, but Sphinx autodoc recognizes a few field 
lists as significant in its parsing, so I prefer using them:

:param x: Denotes the parameter X. Do not use type information in the 
string, we rely on mypy for that now.

:raises y: explains a case in which an Exception type y may be raised 
either directly by this code or anticipated to be allowed to be raised 
by a helper call. (There's no standard here that I am aware of. I use my 
judgment. Always document direct raise calls, but use your judgment for 
sub-calls.)

:returns: explains the semantics of the return value.

That said, literally any sphinx/ReST markup is OK as long as it passes 
make sphinxdocs. Some sphinx markup is prohibited, like adding new 
full-throated sections. You can use arbitrary field lists, definition 
lists, pre-formatted text, examples, code blocks, whatever.

In general, you are not going to find the kind of semantic validation 
you want to ensure that the parameter names are correct, or that you 
spelled :param: right, or that you didn't miss a parameter or an 
exception. None of that tooling exists for Python.

Thus, it's all rather subjective. No right answers, no validation tools. 
Just whatever seems reasonable to a human eye until such time we 
actually decide to pursue publishing the API docs in the development 
manual, if indeed we ever do so at all.

That series sounds like a great opportunity to hash this all out. That 
is when I would like to remove --missing-docstring, too. There will 
absolutely be a "docstring series" in the future, but I am insisting 
stubbornly it happen after strict typing.

> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/gen.py    | 6 ++++--
> >   scripts/qapi/parser.py | 9 +++++----
> >   2 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index ca66c82b5b8..fc19b2aeb9b 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -154,9 +154,11 @@ def _bottom(self):
> >   
> >   @contextmanager
> >   def ifcontext(ifcond, *args):
> > -    """A 'with' statement context manager to wrap with start_if()/end_if()
> > +    """
> > +    A 'with' statement context manager that wraps with `start_if` and `end_if`.
>
> Sadly, the fact that start_if() and end_if() are functions isn't
> immediately obvious anymore.
>
> I've seen :func:`start_if` elsewhere.  Is this something we should or
> want to use?

We *could*.

`start_if` relies on the default role, which I have provisionally set to 
"any" here, so this is shorthand for :any:`start_if`.

The :any: role means: "cross-reference any type of thing." If there is 
not exactly one thing that matches, it results in an error during the 
documentation build.

I like this, because it's nice short-hand syntax that I think 
communicates effectively to the reader that this is a symbol of some 
kind without needing a premium of ReST-ese.

CONSTANTS are capitalized, Classes are title cased, and functions are 
lower_case. `lower_case` references can be assumed to be functions, but 
I will admit that this is not enforced or necessarily true as we add 
more cross reference types in the future.

(I am trying to add QMP cross-reference syntax!)

I still prefer `start_if` to :func:`start_if` simply because it's less 
markup and is easier to read in plaintext contexts. You're right, it 
doesn't look like a function anymore.

I'm not sure if another annotations would work -- `start_if`() or 
`start_if()`. Both seem kind of clunky to me, to be honest. Personal 
feeling is "not really worth the hassle."

> >   
> > -    *args: any number of QAPIGenCCode
> > +    :param ifcond: List of conditionals
> > +    :param args: any number of `QAPIGenCCode`.
> >   
> >       Example::
> >   
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 9d1a3e2eea9..02983979965 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -381,10 +381,11 @@ def append(self, line):
> >   
> >           The way that the line is dealt with depends on which part of
> >           the documentation we're parsing right now:
> > -        * The body section: ._append_line is ._append_body_line
> > -        * An argument section: ._append_line is ._append_args_line
> > -        * A features section: ._append_line is ._append_features_line
> > -        * An additional section: ._append_line is ._append_various_line
> > +
> > +         * The body section: ._append_line is ._append_body_line
> > +         * An argument section: ._append_line is ._append_args_line
> > +         * A features section: ._append_line is ._append_features_line
> > +         * An additional section: ._append_line is ._append_various_line
> >           """
> >           line = line[1:]
> >           if not line:
>
> I understand why you insert a blank line (reST wants blank lines around
> lists), I don't understand why you indent.  Can you explain?

I was mistaken about it needing the indent!

--js