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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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.

I ask to make sure I understand goals and limitations of your doc string
work in this series.

Also, even a passing to Sphinx becomes more useful when accompanied by a
link to relevant documentation.

> 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/

I find PEP 257 frustrating.  It leaves me with more questions than
answers.

> 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.

This paragraph is already not PEP 257.

> """
>
> 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.

I prefer the symmetric form myself.

> 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:

Doc link?

> :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.

Okay.  Nevertheless, I'd prefer a bit more information in the commit
message.  Here's my try:

    qapi: Modify docstrings to be sphinx-compatible

    I did not say "sphinx beautiful", just "sphinx compatible". They
    will not throw errors when parsed and interpreted as ReST.  Finding
    a comprehensive style guide for our docstring utilization is left
    for another day.

    For now, use field lists recognized by Sphinx autodoc.
    FIXME link to their documentation

>
>>>
>>> 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,

`lower_case` could also refer to an attribute, variable, or parameter.

> 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.

Yes, :func:`start_if` is rather heavy.  I asked because I wanted to
understand what :func: buys us.  Not meant as endorsement.

GDK-Doc seems smart enough to recognize start_if().  Sphinx isn't,
because it's built around reST syntax.  We put our money on the Sphinx
horse, so...

> 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."

You later reported the latter works.

I prefer `start_if()` to `start_if`.  Matter of taste.

>
>>>   -    *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!

Easy enough to tidy up :)