diff mbox series

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

Message ID 20201005195158.2348217-3-jsnow@redhat.com
State New, archived
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC
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.
- 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
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(-)

Comments

Markus Armbruster Oct. 6, 2020, 11:21 a.m. UTC | #1
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?

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

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?
John Snow Oct. 6, 2020, 3:23 p.m. UTC | #2
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.

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

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

--js
Markus Armbruster Oct. 7, 2020, 7:24 a.m. UTC | #3
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.

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

>> 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.
John Snow Oct. 7, 2020, 5 p.m. UTC | #4
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.
>
diff mbox series

Patch

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