On Fri, Feb 7, 2025, 6:57 AM Markus Armbruster <arm...@redhat.com> wrote:
> Daniel P. Berrangé <berra...@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é <berra...@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) > > > > [...] > >
