Re: [PATCH v6 19/19] qapi/introspect.py: add SchemaMetaType enum

[John Snow <jsnow@redhat.com>]

On 2/16/21 4:24 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Follows the qapi/introspect.py definition of the same; this adds a more
>> precise typing to _gen_tree's mtype parameter.
>>
>> NB: print(SchemaMetaType.BUILTIN) would produce the string
>> "SchemaMetaType.BUILTIN", but when using format strings (.format or f-strings),
>> it relies on the __format__ method defined in the Enum class, which uses the
>> "value" of the enum instead, producing the string "builtin".
>>
>> For consistency with old-style format strings (which simply call the
>> __str__ method of an object), a __str__ dunder is added, though it is
>> not actually used here in this code.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 38 +++++++++++++++++++++++++++++---------
>>   1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index c6f5cf8d874..008a21f5c4c 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -11,6 +11,7 @@
>>   See the COPYING file in the top-level directory.
>>   """
>>   
>> +from enum import Enum
>>   from typing import (
>>       Any,
>>       Dict,
>> @@ -79,6 +80,23 @@
>>   SchemaInfoCommand = Dict[str, object]
>>   
>>   
>> +class SchemaMetaType(str, Enum):
>> +    """
>> +    Mimics the SchemaMetaType enum from qapi/introspect.json.
>> +    """
>> +    BUILTIN = 'builtin'
>> +    ENUM = 'enum'
>> +    ARRAY = 'array'
>> +    OBJECT = 'object'
>> +    ALTERNATE = 'alternate'
>> +    COMMAND = 'command'
>> +    EVENT = 'event'
>> +
>> +    def __str__(self) -> str:
>> +        # Needed for intuitive behavior with old-style format strings.
>> +        return str(self.value)
>> +
>> +
> 
> The fanciness compared to plain Enum('SchemaMetaType', 'BUILTIN ...')
> avoids extra code to map the enum values to the strings with need.
> 

I wasn't even aware there was a short form. (TIL!)

This form allows me to inherit from str and pass the value around 
anywhere strings are used. Due to the generalized nature of 
tree_to_qlit, using the short constructor form (which creates ints) 
would need additional magic to be useful.

You can almost replicate it:

_values = ('builtin', 'enum', 'array')
_nv_pairs = [(value.upper(), value) for value in _values]
SchemaMetaType = enum.Enum('SchemaMetaType', _nv_pairs, type=str)

though this loses out on the __str__ method hack, and I don't think mypy 
will be able to introspect into this functional constructor.

>>   _ValueT = TypeVar('_ValueT', bound=_Value)
>>   
>>   
>> @@ -251,7 +269,8 @@ def _gen_features(features: Sequence[QAPISchemaFeature]
>>                         ) -> List[Annotated[str]]:
>>           return [Annotated(f.name, f.ifcond) for f in features]
>>   
>> -    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>> +    def _gen_tree(self, name: str, mtype: SchemaMetaType,
>> +                  obj: Dict[str, object],
>>                     ifcond: Sequence[str] = (),
>>                     features: Sequence[QAPISchemaFeature] = ()) -> None:
>>           """
>> @@ -299,7 +318,7 @@ def _gen_variant(self, variant: QAPISchemaVariant
>>   
>>       def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>>                              json_type: str) -> None:
>> -        self._gen_tree(name, 'builtin', {'json-type': json_type})
>> +        self._gen_tree(name, SchemaMetaType.BUILTIN, {'json-type': json_type})
>>   
>>       def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>>                           ifcond: Sequence[str],
>> @@ -307,7 +326,7 @@ def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>>                           members: List[QAPISchemaEnumMember],
>>                           prefix: Optional[str]) -> None:
>>           self._gen_tree(
>> -            name, 'enum',
>> +            name, SchemaMetaType.ENUM,
>>               {'values': [Annotated(m.name, m.ifcond) for m in members]},
>>               ifcond, features
>>           )
>> @@ -316,8 +335,8 @@ def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>>                            ifcond: Sequence[str],
>>                            element_type: QAPISchemaType) -> None:
>>           element = self._use_type(element_type)
>> -        self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>> -                       ifcond)
>> +        self._gen_tree('[' + element + ']', SchemaMetaType.ARRAY,
>> +                       {'element-type': element}, ifcond)
>>   
>>       def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>                                  ifcond: Sequence[str],
>> @@ -330,14 +349,14 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>           if variants:
>>               obj['tag'] = variants.tag_member.name
>>               obj['variants'] = [self._gen_variant(v) for v in variants.variants]
>> -        self._gen_tree(name, 'object', obj, ifcond, features)
>> +        self._gen_tree(name, SchemaMetaType.OBJECT, obj, ifcond, features)
>>   
>>       def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>>                                ifcond: Sequence[str],
>>                                features: List[QAPISchemaFeature],
>>                                variants: QAPISchemaVariants) -> None:
>>           self._gen_tree(
>> -            name, 'alternate',
>> +            name, SchemaMetaType.ALTERNATE,
>>               {'members': [Annotated({'type': self._use_type(m.type)},
>>                                      m.ifcond)
>>                            for m in variants.variants]},
>> @@ -361,7 +380,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>>           }
>>           if allow_oob:
>>               obj['allow-oob'] = allow_oob
>> -        self._gen_tree(name, 'command', obj, ifcond, features)
>> +        self._gen_tree(name, SchemaMetaType.COMMAND, obj, ifcond, features)
>>   
>>       def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>>                       ifcond: Sequence[str], features: List[QAPISchemaFeature],
>> @@ -370,7 +389,8 @@ def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>>           assert self._schema is not None
>>   
>>           arg_type = arg_type or self._schema.the_empty_object_type
>> -        self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
>> +        self._gen_tree(name, SchemaMetaType.EVENT,
>> +                       {'arg-type': self._use_type(arg_type)},
>>                          ifcond, features)
> 
> Gain: _gen_tree()'s second argument's type now serves as documentation,
> and passing crap to it becomes harder.
> 
> Gut feeling: too much notational overhead for too little gain.
> 
> Opinions?
> 

No strong feelings one way or the other, honestly. I wrote this mostly 
to see how much the overhead would be based on your comment about the 
loose typing of meta-type as str.

If it's too much for too little for you, I'm okay without it too.

--js