On Fri, Feb 7, 2025, 5:30 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes:
>
> > On Fri, Jan 31, 2025 at 8:18 AM Markus Armbruster <arm...@redhat.com>
> wrote:
> >
> >> Cc: John Snow for Python typing expertise.
> >>
> >> 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>
>
> [...]
>
> >> > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> >> > new file mode 100644
> >> > index 0000000000..f32f9fe5f4
> >> > --- /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
> >> > +
> >> > +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: Dict[str, QAPISchemaFeature] = {}
> >> > +
> >> > + def visit_begin(self, schema: QAPISchema) -> None:
> >> > + self.features = schema.features()
> >>
> >> Inconsistent type hints:
> >>
> >> $ mypy --config-file scripts/qapi/mypy.ini scripts/qapi-gen.py
> >> scripts/qapi/schema.py:1164: error: Incompatible return value type
> >> (got "dict_values[str, QAPISchemaFeature]", expected
> >> "List[QAPISchemaFeature]") [return-value]
> >> scripts/qapi/features.py:31: error: Incompatible types in assignment
> >> (expression has type "List[QAPISchemaFeature]", variable has type
> >> "Dict[str, QAPISchemaFeature]") [assignment]
> >>
> >> We've been working towards having the build run mypy, but we're not
> >> there, yet. Sorry for the inconvenience!
> >>
> >> schema.features() returns .values(), i.e. a view object.
> >>
> >> I guess the type hint should be ValuesView[QAPISchemaFeature], both for
> >> type type of attribute .features above, and for the return type of
> >> method .features() below. John?
> >>
> >
> > It's probably easiest to just use list(...) in the return and then use
> > List[T] anywhere it matters. The values view type is "kind of, but not
> > actually a list" because it isn't mutable. It is, however, an
> > Iterable/Sequence. You can either convert it to a list or make the typing
> > more abstract.
> >
> > (Rule of thumb: return types should be as specific as possible, input
> types
> > should be as abstract as possible.)
>
> Converting a view to a list makes a copy, right?
>
> I'm not asking because that would be terrible. I just like to
> understand things.
>
> I'd like to move further discussion to Daniel's v4.
'Kay, but let me answer your direct questions here, sorry. I'll switch to
v4 afterwards.
Yeah, list(iterable) just builds a list from the iterable, so it uses the
iterable, immutable "view" into the dict keys to build a list. The dict
view is a "live" object attached to the dict, the list is a static mutable
list fixed at time of copy.
(you could type it more accurately, but that can be annoying, so you can
just convert it to something "normal" like a list or tuple.)
> > I apologize for this format of relaying patches as it is against the
> blood
> > oath I swore as a maintainer, but it's late in my day, forgive me:
> > https://gitlab.com/jsnow/qemu/-/commits/dan-fixup
> >
> > That branch has two things in it:
> >
> > (1) patches to make the python/ tests check the qapi module. This means
> the
> > "make check-minreqs" test you can run from python/ will be run by the
> > GitLab pipelines. You can also run "make check-tox" manually, or run the
> > optional python-tox test from the pipeline dashboard.
>
> These are:
>
> dd9e47f0a8 qapi: update pylintrc config
> dfc6344f32 python: add qapi static analysis tests
> 1f89bf53ed qapi: delete un-needed static analysis configs
>
> Will you post them for review & merging?
>
Yep! Just wanted to offer them as part of this fixup/review to make it easy
for the both of you to run tests consistently. I know it's been a real PITA
to lint qapi, and I found a really simple way to ease that pain and wanted
to share right away.
> > (2) two fixups for linting problems with this series with my s-o-b; feel
> > free to steal them if they're good enough for you.
>
> These are:
>
> b36a412162 fixup
> fa6c90ea76 fixup
>
> I'll post them in reply to Daniel's v4 so they get recorded in the list
> archive.
>
Thanks, didn't realize there was a v4 since I'm not in the CC list there.
Add me to the replies if you have further questions or requests for advice.
> > Thank you for your patience,
> > --js
>
> Thanks for your help!
>
> [...]
>
>
