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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

Yep.  Frustrating.

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

More reason to provide a link!

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

Agree.

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

"Return such-and-such when..." is just as correct: imperative mood.  I
prefer imperative mood for function contracts.

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

Sounds useful, but yes, it's clearly outside QEMU context.

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

Makes sense.

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

How would you mark up variable names in doc strings?  Often, their
"variableness" is obvious from context, but not always.  In C comments,
we tend to use @var [*].

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

Are both equally available in Python IDEs and in interactive Python?

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

Thanks!

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

Thanks again!


[*] GTK-Doc says @var is just for parameters, but since it offers
nothing for variables, we sometimes use it for variables as well.