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

[John Snow <jsnow@redhat.com>]

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.