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

[John Snow <jsnow@redhat.com>]

On 10/1/20 4:52 AM, Markus Armbruster wrote:
> 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.
> 

Yeah, sorry. That's what we're dealing with. It's very open-ended.

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

Right -- well, it isn't NOT PEP 257 either. It just suggests you have to 
describe these features, but it doesn't say HOW.

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

Hard to search for in my opinion; you want to search for "sphinx python 
domain", and then click on "field lists" on the sidebar.

https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists

It has a special understanding for:

param/parameter/arg/argument/key/keyword: I prefer "param" here. 
Possibly key/keyword if we use a **kwargs form with a key that we 
specially recognize, but I've not tested that yet. I know pycharm 
understands "param" in a semantic way, and that's been good enough for me.

type: Defines the type of a parameter. In my opinion, do not use this. 
Let native type hints do the lifting.

raises/raise/except/exception: I prefer "raises". "raises ErrorType 
when...." is a good sentence.

var/ivar/cvar: Describes a variable, presumably in the body of the 
function below. I've never used this, I always describe it in prose instead.

vartype: Defines a type for a variable; I would again defer to the 
native type system instead now.

returns/return: I prefer "returns" for grammatical reasons again. 
("Returns such-and-such when...")

rtype: again, type information. Don't use.

meta: For directives to sphinx, e.g. :meta private: or :meta public: to 
toggle the visibility class from its default. I don't use this.


None of these are validated or checked in any meaningful way; you can 
use arbitrarily field lists (and I do in a few places!) to define your 
own terms and so on.


(I would like to improve autodoc in the future to validate your 
docstrings such that you can enforce :param:, :raises: and :return: and 
it uses the type hints and introspection information to raise an error 
when you make an obvious mistake. I am not there yet, but I am using 
Peter Maydell's work to help inform how I might write such an extension 
to autodoc. This work is not critical, but it will likely occur 
upstream, outside of the QEMU context because I believe this is a good 
thing to do for the ecosystem in general, to allow autodoc to function 
slightly more like e.g. Doxygen does.)

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

That I can do -- and I will double down on my IOU for a more formal 
style guide: https://gitlab.com/jsnow/qemu/-/issues/7

I didn't bother writing it in any of the commits because I felt like 
it'd get lost there and would be mostly useless; but a .rst doc inside 
the package folder would be hard to miss.

I plan to check in something like ./python/README.rst or 
./python/CODING_STYLE.rst to try and formalize a lot of what I am doing 
here, where it's going to be harder to miss.

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

Hm. Attribute yes, actually. variable and parameter no -- sphinx does 
not presently provide syntax or roles for creating anchors to parameter 
names or variables, so they are not able to be cross-referenced.

Attributes CAN be cross-referenced, but only when they are documented.

Another style guide thing:

#: x is a number that represents "The Answer". See `Douglas Adams`_.
self.x = 42

You can use the special comment form "#:" to add a one-line description 
of an attribute that Sphinx will pick up. Sphinx skips these attributes 
otherwise. If you consider them part of the interface of the module, 
it's maybe a good idea to do this.

You can also use docstrings, but the ordering changes:

self.x = 42
"""x is a number that represents "The Answer". See `Douglas Adams`_.

I kind of like the #: form because it announces what follows, but I 
admit it's a bit of special sphinx magic.

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

It specifically targets only cross-references of that exact type. In the 
case that the :any: reference is ambiguous, :func: is the disambiguation.

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

Change made.

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

Already done!