Re: [PATCH v4 4/4] qapi: expose all schema features to code
John Snow writes: > On Fri, Feb 7, 2025, 6:57 AM Markus Armbruster wrote: > >> Daniel P. Berrangé 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é >> >> 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.) Aside: John later shared a useful technique on IRC: "you can write reveal_type(foo) to get mypy to spill the beans on what it thinks". > 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. Yes. >I doubt it matters, but you should know > about that. I believe it's just fine. > 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. Since the typing in Daniel's patch is fine, I'll refrain from messing with it. > 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. Thanks!
Re: [PATCH v4 4/4] qapi: expose all schema features to code
On Fri, Feb 7, 2025, 6:57 AM Markus Armbruster wrote: > Daniel P. Berrangé 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é > > 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 00..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) > > > > [...] > >
Re: [PATCH v4 4/4] qapi: expose all schema features to code
Daniel P. Berrangé 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é 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? [...] > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py > new file mode 100644 > index 00..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) > [...]
Re: [PATCH v4 4/4] qapi: expose all schema features to code
Daniel P. Berrangé 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é John pointed out that isort wants the appended fixup. Not thrilled about the additional churn in the first hunk, but let's not fight our tools. Thanks, John! diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py index be3e5d03ff..16ed26672b 100644 --- a/scripts/qapi/features.py +++ b/scripts/qapi/features.py @@ -11,10 +11,7 @@ from .common import c_enum_const, c_name from .gen import QAPISchemaMonolithicCVisitor -from .schema import ( -QAPISchema, -QAPISchemaFeature, -) +from .schema import QAPISchema, QAPISchemaFeature class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor): diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py index 2b9a2c0c02..324081b9fc 100644 --- a/scripts/qapi/main.py +++ b/scripts/qapi/main.py @@ -15,10 +15,10 @@ from .common import must_match from .error import QAPIError from .events import gen_events +from .features import gen_features from .introspect import gen_introspect from .schema import QAPISchema from .types import gen_types -from .features import gen_features from .visit import gen_visit
