Re: [PATCH v4 4/4] qapi: expose all schema features to code

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

On Fri, Feb 7, 2025, 6:57 AM Markus Armbruster <armbru@redhat.com> wrote:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > This replaces use of the constants from the QapiSpecialFeatures
> > enum, with constants from the auto-generate QapiFeatures enum
> > in qapi-features.h
> >
> > The 'deprecated' and 'unstable' features still have a little bit of
> > special handling, being force defined to be the 1st + 2nd features
> > in the enum, regardless of whether they're used in the schema. This
> > retains compatibility with common code that references the features
> > via the QapiSpecialFeatures constants.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Daniel, feel free to ignore this at least for now.  I'm trying to learn
> some typing lore from John.
>
> v3 made mypy unhappy.  I asked John for advice, and also posted a
> solution involving ValuesView I hacked up myself.  Daniel took it for
> v4.
>
> John suggested to use List.
>
> I now wonder whether could use Iterable.
>
> I'll show the three solutions inline.
>
> John, thoughts?
>

ValuesView works just fine. It accurately describes what that function
returns. I only avoided it in my fixup because it's a more obscure type and
generally list is easier to work with as a first-class built in primitive
type to the language.

(read as: I didn't have to consult any docs to fix it up using List and I'm
lazy.)

Your solution describes precisely the type being returned (always good) and
avoids any re-copying of data.

Do be aware by caching the values view object in another object that you
are keeping a "live reference" to the list of dict values that I think can
change if the source dict changes. I doubt it matters, but you should know
about that.

The only design consideration you have now is what type you actually want
to return and why. I think it barely matters, and I'm always going to opt
for whatever is the least annoying for the patch author so I don't have to
bore/torture them with python minutiae.

As long as the tests pass (my first three patches in the dan-fixup branch I
posted based on v3) I'm more than content.


> [...]
>
> > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> > new file mode 100644
> > index 0000000000..be3e5d03ff
> > --- /dev/null
> > +++ b/scripts/qapi/features.py
> > @@ -0,0 +1,51 @@
> > +"""
> > +QAPI features generator
> > +
> > +Copyright 2024 Red Hat
> > +
> > +This work is licensed under the terms of the GNU GPL, version 2.
> > +# See the COPYING file in the top-level directory.
> > +"""
> > +
> > +from typing import Dict, ValuesView
> > +
> > +from .common import c_enum_const, c_name
> > +from .gen import QAPISchemaMonolithicCVisitor
> > +from .schema import (
> > +    QAPISchema,
> > +    QAPISchemaFeature,
> > +)
> > +
> > +
> > +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
> > +
> > +    def __init__(self, prefix: str):
> > +        super().__init__(
> > +            prefix, 'qapi-features',
> > +            ' * Schema-defined QAPI features',
> > +            __doc__)
> > +
> > +        self.features: ValuesView[QAPISchemaFeature]
>
> This is the ValuesView solution.
>
> The List solution:
>
>            self.features: List[QAPISchemaFeature] = []
>
> The Iterable solution:
>
>            self.features: Iterable[QAPISchemaFeature]
>
> [...]
>
>
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index e97c978d38..7f70969c09 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
>
> [...]
>
> > @@ -1147,6 +1161,9 @@ def __init__(self, fname: str):
> >          self._def_exprs(exprs)
> >          self.check()
> >
> > +    def features(self) -> ValuesView[QAPISchemaFeature]:
> > +        return self._feature_dict.values()
>
> This is the ValuesView solution.
>
> The List solution:
>
>        def features(self) -> List[QAPISchemaFeature]:
>            return list(self._feature_dict.values())
>
> The Iterable solution:
>
>        def features(self) -> Iterable[QAPISchemaFeature]:
>            return self._feature_dict.values()
>
>
> > +
> >      def _def_entity(self, ent: QAPISchemaEntity) -> None:
> >          self._entity_list.append(ent)
> >
>
> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 5927 bytes --]