[v5,16/36] qapi/common.py: Convert comments into docstrings, and elaborate

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC

As docstrings, they'll show up in documentation and IDE help.

The docstring style being targeted is the Sphinx documentation
style. Sphinx uses an extension of ReST with "domains". We use the
(implicit) Python domain, which supports a number of custom "info
fields". Those info fields are documented here:
https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists

Primarily, we use `:param X: descr`, `:return[s]: descr`, and `:raise[s]
Z: when`. Everything else is the Sphinx dialect of ReST.

(No, nothing checks or enforces this style that I am aware of. Sphinx
either chokes or succeeds, but does not enforce a standard of what is
otherwise inside the docstring. Pycharm does highlight when your param
fields are not aligned with the actual fields present. It does not
highlight missing return or exception statements. There is no existing
style guide I am aware of that covers a standard for a minimally
acceptable docstring. I am debating writing one.)

Signed-off-by: John Snow <[e-mail]>
Reviewed-by: Eduardo Habkost <[e-mail]>
Reviewed-by: Cleber Rosa <[e-mail]>
---
 scripts/qapi/common.py | 53 +++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

Comments

Markus Armbruster Oct. 7, 2020, 9:14 a.m. UTC | #1

John Snow <[e-mail]> writes:

> As docstrings, they'll show up in documentation and IDE help.
>
> The docstring style being targeted is the Sphinx documentation
> style. Sphinx uses an extension of ReST with "domains". We use the
> (implicit) Python domain, which supports a number of custom "info
> fields". Those info fields are documented here:
> https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists
>
> Primarily, we use `:param X: descr`, `:return[s]: descr`, and `:raise[s]
> Z: when`. Everything else is the Sphinx dialect of ReST.
>
> (No, nothing checks or enforces this style that I am aware of. Sphinx
> either chokes or succeeds, but does not enforce a standard of what is
> otherwise inside the docstring. Pycharm does highlight when your param
> fields are not aligned with the actual fields present. It does not
> highlight missing return or exception statements. There is no existing
> style guide I am aware of that covers a standard for a minimally
> acceptable docstring. I am debating writing one.)
>
> Signed-off-by: John Snow <[e-mail]>
> Reviewed-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Cleber Rosa <[e-mail]>
> ---
>  scripts/qapi/common.py | 53 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 74a2c001ed9..0ef38ea5fe0 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -15,15 +15,24 @@
>  from typing import Optional, Sequence
>  
>  
> +#: Sentinel value that causes all space to its right to be removed.

What's the purpose of : after # ?

I'm not sure this is a "sentinel value".  Wikipedia:

    In computer programming, a sentinel value (also referred to as a
    flag value, trip value, rogue value, signal value, or dummy data)[1]
    is a special value in the context of an algorithm which uses its
    presence as a condition of termination, typically in a loop or
    recursive algorithm.

    https://en.wikipedia.org/wiki/Sentinel_value

Perhaps

   # Magic string value that gets removed along with all space to the
   # right.

>  EATSPACE = '\033EATSPACE.'
>  POINTER_SUFFIX = ' *' + EATSPACE
>  _C_NAME_TRANS = str.maketrans('.-', '__')
>  
>  
> -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
> -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
> -# ENUM24_Name -> ENUM24_NAME
>  def camel_to_upper(value: str) -> str:
> +    """
> +    Converts CamelCase to CAMEL_CASE.
> +
> +    Examples:
> +      ENUMName -> ENUM_NAME
> +      EnumName1 -> ENUM_NAME1
> +      ENUM_NAME -> ENUM_NAME
> +      ENUM_NAME1 -> ENUM_NAME1
> +      ENUM_Name2 -> ENUM_NAME2
> +      ENUM24_Name -> ENUM24_NAME
> +    """

I wonder whether these indented lines get wrapped into one
unintelligible parapgraph.

Have you eyeballed the output of Sphinx?

>      c_fun_str = c_name(value, False)
>      if value.isupper():
>          return c_fun_str
> @@ -45,21 +54,33 @@ def camel_to_upper(value: str) -> str:
>  def c_enum_const(type_name: str,
>                   const_name: str,
>                   prefix: Optional[str] = None) -> str:
> +    """
> +    Generate a C enumeration constant name.
> +
> +    :param type_name: The name of the enumeration.
> +    :param const_name: The name of this constant.
> +    :param prefix: Optional, prefix that overrides the type_name.
> +    """
>      if prefix is not None:
>          type_name = prefix
>      return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>  
>  
> -# Map @name to a valid C identifier.
> -# If @protect, avoid returning certain ticklish identifiers (like
> -# C keywords) by prepending 'q_'.
> -#
> -# Used for converting 'name' from a 'name':'type' qapi definition
> -# into a generated struct member, as well as converting type names
> -# into substrings of a generated C function name.
> -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
> -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>  def c_name(name: str, protect: bool = True) -> str:
> +    """
> +    Map ``name`` to a valid C identifier.
> +
> +    Used for converting 'name' from a 'name':'type' qapi definition
> +    into a generated struct member, as well as converting type names
> +    into substrings of a generated C function name.
> +
> +    '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
> +    protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
> +
> +    :param name: The name to map.
> +    :param protect: If true, avoid returning certain ticklish identifiers
> +                    (like C keywords) by prepending ``q_``.
> +    """
>      # ANSI X3J11/88-090, 3.1.1
>      c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>                       'default', 'do', 'double', 'else', 'enum', 'extern',
> @@ -129,12 +150,16 @@ def decrease(self, amount: int = 4) -> None:
>          self._level -= amount
>  
>  
> +#: Global, current indent level for code generation.
>  indent = Indentation()
>  
>  
> -# Generate @code with @kwds interpolated.
> -# Obey indent, and strip EATSPACE.
>  def cgen(code: str, **kwds: object) -> str:
> +    """
> +    Generate ``code`` with ``kwds`` interpolated.
> +
> +    Obey `indent`, and strip `EATSPACE`.
> +    """
>      raw = code % kwds
>      if indent:
>          raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE)

John Snow Oct. 7, 2020, 3:23 p.m. UTC | #2

On 10/7/20 5:14 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> As docstrings, they'll show up in documentation and IDE help.
>>
>> The docstring style being targeted is the Sphinx documentation
>> style. Sphinx uses an extension of ReST with "domains". We use the
>> (implicit) Python domain, which supports a number of custom "info
>> fields". Those info fields are documented here:
>> https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists
>>
>> Primarily, we use `:param X: descr`, `:return[s]: descr`, and `:raise[s]
>> Z: when`. Everything else is the Sphinx dialect of ReST.
>>
>> (No, nothing checks or enforces this style that I am aware of. Sphinx
>> either chokes or succeeds, but does not enforce a standard of what is
>> otherwise inside the docstring. Pycharm does highlight when your param
>> fields are not aligned with the actual fields present. It does not
>> highlight missing return or exception statements. There is no existing
>> style guide I am aware of that covers a standard for a minimally
>> acceptable docstring. I am debating writing one.)
>>
>> Signed-off-by: John Snow <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> ---
>>   scripts/qapi/common.py | 53 +++++++++++++++++++++++++++++++-----------
>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index 74a2c001ed9..0ef38ea5fe0 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>> @@ -15,15 +15,24 @@
>>   from typing import Optional, Sequence
>>   
>>   
>> +#: Sentinel value that causes all space to its right to be removed.
> 
> What's the purpose of : after # ?
> 

Documents this name in Sphinx. We had a small discussion about it, I 
think; "Does using this special form or the docstring make the comment 
visible in any IDE?" (No.)

There's no Python-AST way to document these, but there is a Sphinx way 
to document them, so I did that.

(Doing it like this allows `EATSPACE` to be used as a cross-reference.)

> I'm not sure this is a "sentinel value".  Wikipedia:
> 
>      In computer programming, a sentinel value (also referred to as a
>      flag value, trip value, rogue value, signal value, or dummy data)[1]
>      is a special value in the context of an algorithm which uses its
>      presence as a condition of termination, typically in a loop or
>      recursive algorithm.
> 
>      https://en.wikipedia.org/wiki/Sentinel_value
> 

I really should try to learn English as a second language so I know what 
any of the words I use mean, I guess. I had slipped to a less strict 
usage where it meant more like "placeholder".

> Perhaps
> 
>     # Magic string value that gets removed along with all space to the
>     # right.
> 

This can be written on one line if we gently disregard the 72 column 
limit. (Maybe you already did when you wrote it and my client wrapped 
it. Who knows!)

>>   EATSPACE = '\033EATSPACE.'
>>   POINTER_SUFFIX = ' *' + EATSPACE
>>   _C_NAME_TRANS = str.maketrans('.-', '__')
>>   
>>   
>> -# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>> -# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>> -# ENUM24_Name -> ENUM24_NAME
>>   def camel_to_upper(value: str) -> str:
>> +    """
>> +    Converts CamelCase to CAMEL_CASE.
>> +
>> +    Examples:
>> +      ENUMName -> ENUM_NAME
>> +      EnumName1 -> ENUM_NAME1
>> +      ENUM_NAME -> ENUM_NAME
>> +      ENUM_NAME1 -> ENUM_NAME1
>> +      ENUM_Name2 -> ENUM_NAME2
>> +      ENUM24_Name -> ENUM24_NAME
>> +    """
> 
> I wonder whether these indented lines get wrapped into one
> unintelligible parapgraph.
> 
> Have you eyeballed the output of Sphinx?
> 

Eyeballed, but didn't validate this specific one. Yeah, it's nonsense.

Examples::

     ENUMName -> ENUM_NAME

etc. works better.

>>       c_fun_str = c_name(value, False)
>>       if value.isupper():
>>           return c_fun_str
>> @@ -45,21 +54,33 @@ def camel_to_upper(value: str) -> str:
>>   def c_enum_const(type_name: str,
>>                    const_name: str,
>>                    prefix: Optional[str] = None) -> str:
>> +    """
>> +    Generate a C enumeration constant name.
>> +
>> +    :param type_name: The name of the enumeration.
>> +    :param const_name: The name of this constant.
>> +    :param prefix: Optional, prefix that overrides the type_name.
>> +    """
>>       if prefix is not None:
>>           type_name = prefix
>>       return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
>>   
>>   
>> -# Map @name to a valid C identifier.
>> -# If @protect, avoid returning certain ticklish identifiers (like
>> -# C keywords) by prepending 'q_'.
>> -#
>> -# Used for converting 'name' from a 'name':'type' qapi definition
>> -# into a generated struct member, as well as converting type names
>> -# into substrings of a generated C function name.
>> -# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>> -# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>>   def c_name(name: str, protect: bool = True) -> str:
>> +    """
>> +    Map ``name`` to a valid C identifier.
>> +
>> +    Used for converting 'name' from a 'name':'type' qapi definition
>> +    into a generated struct member, as well as converting type names
>> +    into substrings of a generated C function name.
>> +
>> +    '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
>> +    protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
>> +
>> +    :param name: The name to map.
>> +    :param protect: If true, avoid returning certain ticklish identifiers
>> +                    (like C keywords) by prepending ``q_``.
>> +    """
>>       # ANSI X3J11/88-090, 3.1.1
>>       c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
>>                        'default', 'do', 'double', 'else', 'enum', 'extern',
>> @@ -129,12 +150,16 @@ def decrease(self, amount: int = 4) -> None:
>>           self._level -= amount
>>   
>>   
>> +#: Global, current indent level for code generation.
>>   indent = Indentation()
>>   
>>   
>> -# Generate @code with @kwds interpolated.
>> -# Obey indent, and strip EATSPACE.
>>   def cgen(code: str, **kwds: object) -> str:
>> +    """
>> +    Generate ``code`` with ``kwds`` interpolated.
>> +
>> +    Obey `indent`, and strip `EATSPACE`.
>> +    """
>>       raw = code % kwds
>>       if indent:
>>           raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE)

Markus Armbruster Oct. 8, 2020, 7:20 a.m. UTC | #3

John Snow <[e-mail]> writes:

> On 10/7/20 5:14 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> writes:
>> 
>>> As docstrings, they'll show up in documentation and IDE help.
>>>
>>> The docstring style being targeted is the Sphinx documentation
>>> style. Sphinx uses an extension of ReST with "domains". We use the
>>> (implicit) Python domain, which supports a number of custom "info
>>> fields". Those info fields are documented here:
>>> https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists
>>>
>>> Primarily, we use `:param X: descr`, `:return[s]: descr`, and `:raise[s]
>>> Z: when`. Everything else is the Sphinx dialect of ReST.
>>>
>>> (No, nothing checks or enforces this style that I am aware of. Sphinx
>>> either chokes or succeeds, but does not enforce a standard of what is
>>> otherwise inside the docstring. Pycharm does highlight when your param
>>> fields are not aligned with the actual fields present. It does not
>>> highlight missing return or exception statements. There is no existing
>>> style guide I am aware of that covers a standard for a minimally
>>> acceptable docstring. I am debating writing one.)
>>>
>>> Signed-off-by: John Snow <[e-mail]>
>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>> ---
>>>   scripts/qapi/common.py | 53 +++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 74a2c001ed9..0ef38ea5fe0 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -15,15 +15,24 @@
>>>   from typing import Optional, Sequence
>>>     
>>> +#: Sentinel value that causes all space to its right to be removed.
>> What's the purpose of : after # ?
>> 
>
> Documents this name in Sphinx. We had a small discussion about it, I
> think; "Does using this special form or the docstring make the comment 
> visible in any IDE?" (No.)
>
> There's no Python-AST way to document these, but there is a Sphinx way
> to document them, so I did that.
>
> (Doing it like this allows `EATSPACE` to be used as a cross-reference.)

Thanks.

Consider pointing this out when you write the comment & doc string part
of our Python style guide.

>> I'm not sure this is a "sentinel value".  Wikipedia:
>>      In computer programming, a sentinel value (also referred to as a
>>      flag value, trip value, rogue value, signal value, or dummy data)[1]
>>      is a special value in the context of an algorithm which uses its
>>      presence as a condition of termination, typically in a loop or
>>      recursive algorithm.
>>      https://en.wikipedia.org/wiki/Sentinel_value
>> 
>
> I really should try to learn English as a second language so I know
> what any of the words I use mean, I guess. I had slipped to a less
> strict usage where it meant more like "placeholder".
>
>> Perhaps
>>     # Magic string value that gets removed along with all space to
>> the
>>     # right.
>> 
>
> This can be written on one line if we gently disregard the 72 column
> limit. (Maybe you already did when you wrote it and my client wrapped 
> it. Who knows!)

Drop the period and it fits ;-P

You could also drop "value" without loss.

[...]

Patch

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 74a2c001ed9..0ef38ea5fe0 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -15,15 +15,24 @@ 
 from typing import Optional, Sequence
 
 
+#: Sentinel value that causes all space to its right to be removed.
 EATSPACE = '\033EATSPACE.'
 POINTER_SUFFIX = ' *' + EATSPACE
 _C_NAME_TRANS = str.maketrans('.-', '__')
 
 
-# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
-# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
-# ENUM24_Name -> ENUM24_NAME
 def camel_to_upper(value: str) -> str:
+    """
+    Converts CamelCase to CAMEL_CASE.
+
+    Examples:
+      ENUMName -> ENUM_NAME
+      EnumName1 -> ENUM_NAME1
+      ENUM_NAME -> ENUM_NAME
+      ENUM_NAME1 -> ENUM_NAME1
+      ENUM_Name2 -> ENUM_NAME2
+      ENUM24_Name -> ENUM24_NAME
+    """
     c_fun_str = c_name(value, False)
     if value.isupper():
         return c_fun_str
@@ -45,21 +54,33 @@  def camel_to_upper(value: str) -> str:
 def c_enum_const(type_name: str,
                  const_name: str,
                  prefix: Optional[str] = None) -> str:
+    """
+    Generate a C enumeration constant name.
+
+    :param type_name: The name of the enumeration.
+    :param const_name: The name of this constant.
+    :param prefix: Optional, prefix that overrides the type_name.
+    """
     if prefix is not None:
         type_name = prefix
     return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper()
 
 
-# Map @name to a valid C identifier.
-# If @protect, avoid returning certain ticklish identifiers (like
-# C keywords) by prepending 'q_'.
-#
-# Used for converting 'name' from a 'name':'type' qapi definition
-# into a generated struct member, as well as converting type names
-# into substrings of a generated C function name.
-# '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
-# protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
 def c_name(name: str, protect: bool = True) -> str:
+    """
+    Map ``name`` to a valid C identifier.
+
+    Used for converting 'name' from a 'name':'type' qapi definition
+    into a generated struct member, as well as converting type names
+    into substrings of a generated C function name.
+
+    '__a.b_c' -> '__a_b_c', 'x-foo' -> 'x_foo'
+    protect=True: 'int' -> 'q_int'; protect=False: 'int' -> 'int'
+
+    :param name: The name to map.
+    :param protect: If true, avoid returning certain ticklish identifiers
+                    (like C keywords) by prepending ``q_``.
+    """
     # ANSI X3J11/88-090, 3.1.1
     c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
                      'default', 'do', 'double', 'else', 'enum', 'extern',
@@ -129,12 +150,16 @@  def decrease(self, amount: int = 4) -> None:
         self._level -= amount
 
 
+#: Global, current indent level for code generation.
 indent = Indentation()
 
 
-# Generate @code with @kwds interpolated.
-# Obey indent, and strip EATSPACE.
 def cgen(code: str, **kwds: object) -> str:
+    """
+    Generate ``code`` with ``kwds`` interpolated.
+
+    Obey `indent`, and strip `EATSPACE`.
+    """
     raw = code % kwds
     if indent:
         raw = re.sub(r'^(?!(#|$))', str(indent), raw, flags=re.MULTILINE)