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

[John Snow]

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