Re: [PATCH v6 15/19] qapi/introspect.py: Add docstrings to _gen_tree and _tree_to_qlit

[John Snow]

On 2/17/21 4:39 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/introspect.py | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 45284af1330..5d4f5e23f7e 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -99,6 +99,15 @@ def __init__(self, value: _ValueT, ifcond: Iterable[str],
> >   def _tree_to_qlit(obj: JSONValue,
> >                     level: int = 0,
> >                     dict_value: bool = False) -> str:
> > +    """
> > +    Convert the type tree into a QLIT C string, recursively.
> > +
> > +    :param obj: The value to convert.
> > +                This value may not be Annotated when dict_value is True.
> > +    :param level: The indentation level for this particular value.
> > +    :param dict_value: True when the value being processed belongs to a
> > +                       dict key; which suppresses the output indent.
> > +    """
> >   
> >       def indent(level: int) -> str:
> >           return level * 4 * ' '
> > @@ -244,6 +253,15 @@ def _gen_features(features: List[QAPISchemaFeature]
> >       def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> >                     ifcond: Sequence[str],
> >                     features: Optional[List[QAPISchemaFeature]]) -> None:
> > +        """
> > +        Build and append a SchemaInfo object to self._trees.
> > +
> > +        :param name: The entity's name.
> > +        :param mtype: The entity's meta-type.
> > +        :param obj: Additional entity fields, as appropriate for the meta-type.
>
> "Additional members", since we're talking about a JSON object.

I thought "field" was also appropriate for JSON, but I suppose the spec 
doesn't use that word. Over time, "field", "member" and "property" have 
become just meaningless word-slurry to me.

OK.

"Additional members as appropriate for the meta-type."

> > +        :param ifcond: Sequence of conditionals that apply to this entity.
> > +        :param features: Optional features field for SchemaInfo.
>
> Likewise.

"Optional features member for SchemaInfo" ?

Sure.

> Sure we want to restate parts of the type ("Sequence of", "Optional") in
> the doc string?

I usually avoid it, but sometimes for non-scalars I found that it read 
better to give a nod to the plural, as in:

[ifcond is a] sequence of conditionals ...

but, yes, I haven't been consistent about it. right below for @obj I 
omit the type of the container.

"Conditionals that apply to this entity" feels almost too terse in 
isolation.

I don't feel like it's a requisite to state the type, but in some cases 
I unconsciously chose to mention the structure.

With regards to "Optional", I use this word specifically to indicate 
parameters that have default values -- distinct from a type that's 
Optional[], which is really actually like Nullable[T] ... If it makes 
you feel better, Guido says he regrets that naming decision. Oops!

I'm probably not consistent about when I decided to write it, though.

Ehm. If it's not harmful to leave it as-is, I think it'd be okay to do 
so. If you prefer a consistency all one way or all the other, I'd have 
to run the vacuum back over the series to check for it.

> > +        """
> >           comment: Optional[str] = None
> >           if mtype not in ('command', 'event', 'builtin', 'array'):
> >               if not self._unmask:
>
> Also: more line-wrapping for PEP 8.

I thought the 72 column limit was for things like comments and docstrings.