Re: [PATCH 14/37] qapi/common.py: Move comments into docstrings

[John Snow]

On 9/17/20 10:37 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > As docstrings, they'll show up in documentation and IDE help.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/common.py | 50 ++++++++++++++++++++++++++++++------------
> >   1 file changed, 36 insertions(+), 14 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index af01348b35..38d380f0a9 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -20,10 +20,18 @@
> >   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 +53,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.
> > +    """
>
> Not actually a move.  Suggest to retitle
>
>      qapi/common: Turn comments in docstrings, and add more

OK.

> >       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',
> > @@ -135,9 +155,11 @@ def pop(self, amount: int = 4) -> int:
> >   INDENT = Indent(0)
> >   
> >   
> > -# Generate @code with @kwds interpolated.
> > -# Obey INDENT level, and strip EATSPACE.
> >   def cgen(code: str, **kwds: Union[str, int]) -> str:
> > +    """
> > +    Generate `code` with `kwds` interpolated.
> > +    Obey `INDENT`, and strip `EATSPACE`.
> > +    """
> >       raw = code % kwds
> >       if INDENT:
> >           raw, _ = re.subn(r'^(?!(#|$))', str(INDENT), raw, flags=re.MULTILINE)
>
> Can you point to documentation on the docstring conventions and markup
> to use?

Short answer: No.

Long answer:

It's actually completely arbitrary, with major competing de-facto 
standards. Their primary function is to be stored to the __doc__ 
attribute of a module/class/method and can be displayed when using the 
interactive function help(...).

https://www.python.org/dev/peps/pep-0257/ covers docstrings only in an 
extremely broad sense. In summary, it asks:

- Use full sentences that end in periods
- Use the triple-double quote style
- multi-line docstrings should have their closing quote on a line by itself
- multi-line docstrings should use a one-sentence summary, a blank line, 
and then a more elaborate description.

It recommends you document arguments, types, return values and types, 
exceptions and so on but does not dictate a format. Two popular 
conventions are the google-style [1] and the NumPy-style [2] docstring 
formats.

I write docstrings assuming we will be using *Sphinx* as our 
documentation tool. Sphinx does not read docstrings at all by default, 
but *autodoc* does. Autodoc assumes your docstrings are written in the 
Sphinx dialect of ReST.

What you really want to look for is the "Python domain" documentation in 
Sphinx: 
https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html

Autodoc will annotate function/method docstrings with "py:function" or 
"py:method" as appropriate, but the actual contents of the block are 
still up to you.

For those, you want to look up the Python domain info field lists that 
are supported by Sphinx, which are here: 
https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#info-field-lists

Taken together with PEP 257, you generally want something like this:

"""
Function f converts uncertainty into happiness.

Function f only works on days that do not end in 'y'. Caution should be 
used when integrating this function into threaded code.

:param uncertainty: All of your doubts.
:raise RuntimeError: When it is a day ending in 'y'.
:return: Your newfound happiness.
"""

I use the single-backtick as the Sphinx-ese "default role" resolver, 
which generally should resolve to a reference to some Python entity. The 
double-backtick is used to do formatted text for things like string 
literals and so on.



Coffee break.



Having said this, I have not found any tool to date that actually 
*checks* these comments for consistency. The pycharm IDE interactively 
highlights them when it senses that you have made a mistake, but that 
cannot be worked into our CI process that I know of - it's a proprietary 
checker.

So right now, they're just plaintext that I am writing to approximate 
the Sphinx style until such time as I enable autodoc for the module and 
fine-tune the actual formatting and so on.

--js



[1] 
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

[2] 
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_numpy.html#example-numpy