Re: [PATCH v5 02/36] qapi: modify docstrings to be sphinx-compatible

[John Snow <jsnow@redhat.com>]

On 10/7/20 3:24 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 10/6/20 7:21 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> A precise style guide and a package-wide overhaul is forthcoming pending
>>>> further discussion and consensus. At present, we are avoiding obvious
>>>> errors that cause sphinx documentation build problems.
>>>>
>>>> A preliminary style guide is loosely based on PEP 257 and Sphinx
>>>> Autodoc. It is chosen for interoperability with our existing Sphinx
>>>> framework, and because it has loose recognition in the Pycharm IDE.
>>>>
>>>> - Use Triple-double quotes (""").
>>>> - Opening and closing quotes appear on their own lines for multi-line docs.
>>>> - A single-sentence summary should be the first line of the docstring.
>>>> - A blank line follows.
>>>> - Further prose, if needed, is written next and can be multiple paragraphs,
>>>>     contain RST markup, etc.
>>>> - The :param x: desc, :returns: desc, and :raises z: desc info fields follow.
>>> Mandatory when they apply?
>>>
>>
>> Subject of debate...
>>
>> - Some people really hate obvious docstring comments.
>> - Some people really like the consistency.
>>
>> Which type of developer am I? Guess it depends on when you ask.
>>
>> Figured we'd hash that out when I go to write a style guide document.
> 
> Fair enough.
> 
> If I stop reading after the first paragraph, the patch matches
> expectations built by the commit message.
> 

(:

> If I speed-read, the first paragraph barely registers, but the second
> makes me slow down, giving me the mistaken idea that this patch is about
> converting to a preliminary style guide.  It's not, it's about getting
> Sphinx errors out of the way.
> 
> I figure you didn't stop after the first paragraph because you felt a
> need to explain why you resolve the "obvious errors" the way you do.
> 
> Perhaps:
> 
>      qapi: modify docstrings to be sphinx-compatible
> 
>      A precise style guide and a package-wide overhaul is forthcoming
>      pending further discussion and consensus. For now, merely avoid
>      obvious errors that cause Sphinx documentation build problems, using a
>      style loosely based on PEP 257 and Sphinx Autodoc. It is chosen for
>      interoperability with our existing Sphinx framework, and because it
>      has loose recognition in the Pycharm IDE.
> 
>      [...]
>     
> 
>>>> - Additional examples, diagrams, or other metadata follows below.
>>>>
>>>> See also:
>>>>
>>>> https://www.python.org/dev/peps/pep-0257/
>>>> https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists
> 
> Blank line here, by convention.
> 

Wonder why my script didn't do that. Eh.

>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    scripts/qapi/gen.py    | 6 ++++--
>>>>    scripts/qapi/parser.py | 1 +
>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>>> index ca66c82b5b8..dc7b94aa115 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()` / `end_if()`.
>>>>    -    *args: any number of QAPIGenCCode
>>>> +    :param ifcond: A list of conditionals, passed to `start_if()`.
>>>> +    :param args: any number of `QAPIGenCCode`.
>>>>          Example::
>>>>    diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>>>> index 9d1a3e2eea9..31bc2e6dca9 100644
>>>> --- a/scripts/qapi/parser.py
>>>> +++ b/scripts/qapi/parser.py
>>>> @@ -381,6 +381,7 @@ 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
>>> I'm asking because you're not adding :param line: here.
>>>
>>
>> Yeah, it's not necessary to test the syntax of what else I've written
>> with sphinx, so I didn't add it. VERY TECHNICALLY this blurb isn't
>> required at all and could be deleted. You can do so if you'd like; it
>> will just show up later in some other patch or series more designed to
>> fix formatting.
> 
> I recommend (but do not demand) to strictly limit this commit to
> "avoiding obvious errors that cause sphinx documentation build
> problems."
> 

OK, I'll drop this bit for now, but I will keep the new annotations for 
ifcontext, because... well. Why do it twice.

>>> Same for several other functions in this file.
>>> In schema.py:
>>>       class QAPISchemaMember:
>>>           """ Represents object members, enum members and features """
>>> Are the spaces next to """ okay?
>>>
>>
>> Ideally cleaned up, but that's not a goal of this patch or series.
> 
> Got it.
>