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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 2/17/21 11:35 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> 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.
>> 
>> Correct.
>> 
>>>                         Over time, "field", "member" and "property" have
>>> become just meaningless word-slurry to me.
>> 
>> Perfectly understandable.
>> 
>>> OK.
>>>
>>> "Additional members as appropriate for the meta-type."
>> 
>> Let's stick in a SchemaInfo for clarity:
>> 
>>          :param obj: Additional SchemaInfo members, as appropriate for
>>                      the meta-type.
>> 
>
> Sure, why not?
>
>>>>> +        :param ifcond: Sequence of conditionals that apply to this 
>>>>> entity.
>>>>> +        :param features: Optional features field for SchemaInfo.
>>>>
>>>> Likewise.
>>>>
>>>
>>> "Optional features member for SchemaInfo" ?
>>>
>>> Sure.
>> 
>> What about
>> 
>>          :param features: The SchemaInfo's features.
>> 
>
> Sure, why not? x2
>
>>>> 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.
>> 
>> Similarly terse, just with SchemaInfo:
>> 
>>          :param ifcond: Conditionals to apply to the SchemaInfo.
>> 
>
> Sure, why not! x3
>
>> Or "Conditionals to guard the SchemaInfo with".  Doesn't read any
>> better, I fear.  Ideas?
>> 
>>> I don't feel like it's a requisite to state the type, but in some cases
>>> I unconsciously chose to mention the structure.
>> 
>> Then let's work with the informal rule not to repeat types, unless where
>> repeating them makes the text easier to read.  Makes sense to you?
>
> Subjectively I was doing this. Maybe half-heartedly. :~)

I'm not criticizing any half-heartedness here.  I just want to get to a
common understanding of how we want to do doc strings.  Preliminary and
somewhat vague is fine; it's *understanding*, not *law*.  "In John's
head" is not fine :)

>> I suspect the answer we'll give in the long term depends on tooling.
>> When all the tools can show you is the doc string, the doc string better
>> includes type information.  But if the tools show you the types,
>> repeating them wastes precious reader bandwidth.
>> 
>
> Yep. Sphinx shows type in the signature, but it doesn't necessarily 
> repeat it for the :param list: entries; So you can correlate it with 
> your eyeballs, but it isn't done for you.
>
> Maybe that'll change. Maybe I'll change it with my own Sphinx plugin 
> eventually that does the cool stuff I dream about.
>
> Maybe Maybe Maybe. I need to study the docutils API and learn how to 
> make even a simple plugin... There are definitely a few ideas that I 
> have that I want to bring to life that I think will help people like me 
> adhere to a more consistent style.
>
>>> 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!
>> 
>> He's right.
>> 
>> The "has default value" kind of optional is not part of the type, thus
>> not covered by the proposed informal rule.  Similar, if separate
>> question: sure we want to restate the (presence of a) default value in
>> the doc string?
>> 
>
> I tend to state what a default is if the default is a special value that 
> implies something else. Like: "The default is to not add this member." I 
> have generally avoided "The default is 3."
>
> I do sometimes say "Defaults to true/false" for boolean options just to 
> add emphasis on "which way" the boolean leans, in case the name doesn't 
> make that clear in isolation.

Documentation should explain defaults.  But what exactly counts as
documentation today?  Just the doc string?  The doc string plus the type
hints?  Plus the default values?

> I haven't been consistent, but I will try to be a bit more conscious 
> about it going forward.
>
> (the expr.py series, up next, is gonna be a playground for docstring 
> style reviews, because I added a ton.)

I feel we should to pick some rules that work for us with the tooling we
have.  Even imperfect rules that aren't enforced automatically should
help us maintain a useful level of consistency.  Also liberate us from
debating the same doc questions over and over.  Liberate *me* from
debating them in my head.

>> Again, the long-term answer will likely depend on tooling.
>> 
>
> If it reads better to you to remove the "Optional, " then go ahead and 
> make those cuts for the time-being, and I can try to hit things with a 
> docstring beautifying beam later when we actually try to develop and 
> publish a manual.
>
> (After my python packaging series and after this QAPI cleanup series.)

Here's what I have in my tree now:

    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
                  ifcond: Sequence[str] = (),
                  features: Sequence[QAPISchemaFeature] = ()) -> None:
        """
        Build and append a SchemaInfo object to self._trees.

        :param name: The SchemaInfo's name.
        :param mtype: The SchemaInfo's meta-type.
        :param obj: Additional SchemaInfo members, as appropriate for
                    the meta-type.
        :param ifcond: Conditionals to apply to the SchemaInfo.
        :param features: The SchemaInfo's features.
        """

PATCH 17 adds

                         Will be omitted from the output if empty.

Hmm, I think that should actually go right into this patch instead.

>>> 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.
>> 
>> Just five patches add comments, and just two doc strings.  I had a look
>> at all of them, and found nothing else in need of vacuuming.
>> 
>
> Go with whatcha feel, but I'll try to write that "style guide" we 
> discussed during part 1 and we can hem and haw over the guidelines for 
> ourselves.
>
> I will want to apply it to all of ./scripts/qapi and ./python/qemu both.

Makes sense.

Perhaps we can steal from existing style guide(s).  Sadly, PEP 257 is
next to useless.

>>>>> +        """
>>>>>            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.
>> 
>> I put this in the wrong spot, I meant the doc string, not the code.
>> sorry for the confusion!
>> 
>
> Ah, phew. OK, yes, I've already capitulated on the comment line lengths, 
> have at those :)