On Thu, Mar 14, 2024, 10:04 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes:
>
> > On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster <arm...@redhat.com>
> wrote:
> >
> >> John Snow <js...@redhat.com> writes:
> >>
> >> > Include entities don't have names, but we generally expect "entities"
> to
> >> > have names. Reclassify all entities with names as *definitions*,
> leaving
> >> > the nameless include entities as QAPISchemaEntity instances.
> >> >
> >> > This is primarily to help simplify typing around expectations of what
> >> > callers expect for properties of an "entity".
> >> >
> >> > Suggested-by: Markus Armbruster <arm...@redhat.com>
> >> > Signed-off-by: John Snow <js...@redhat.com>
> >> > ---
> >> > scripts/qapi/schema.py | 144
> +++++++++++++++++++++++------------------
> >> > 1 file changed, 82 insertions(+), 62 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index 117f0f78f0c..da273c1649d 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >>
> >> [...]
> >>
> >> > @@ -107,6 +90,46 @@ def _set_module(self, schema, info):
> >> > def set_module(self, schema):
> >> > self._set_module(schema, self.info)
> >> >
> >> > + def visit(self, visitor):
> >> > + # pylint: disable=unused-argument
> >> > + assert self._checked
> >> > +
> >> > +
> >> > +class QAPISchemaDefinition(QAPISchemaEntity):
> >> > + meta: Optional[str] = None
> >> > +
> >> > + def __init__(self, name: str, info, doc, ifcond=None,
> features=None):
> >> > + assert isinstance(name, str)
> >> > + super().__init__(info)
> >> > + for f in features or []:
> >> > + assert isinstance(f, QAPISchemaFeature)
> >> > + f.set_defined_in(name)
> >> > + self.name = name
> >> > + self.doc = doc
> >> > + self._ifcond = ifcond or QAPISchemaIfCond()
> >> > + self.features = features or []
> >> > +
> >> > + def __repr__(self):
> >> > + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
> >> > + id(self))
> >> > +
> >> > + def c_name(self):
> >> > + return c_name(self.name)
> >> > +
> >> > + def check(self, schema):
> >> > + assert not self._checked
> >> > + seen = {}
> >> > + for f in self.features:
> >> > + f.check_clash(self.info, seen)
> >> > + super().check(schema)
> >>
> >> v3 called super().check() first. Why did you move it? I'm asking just
> >> to make sure I'm not missing something subtle.
> >>
> >
> > I don't believe you are, I think it was just ... something I didn't
> notice
> > when rebasing, since I merged this patch by hand. I recall having to
> insert
> > the super call manually, and I guess my preferences are nondeterministic.
>
> I'd like to move it back, for consistency with its subtypes' .check(),
> which all call super().check() early. Okay?
>
Yes, please do.
(You *really* don't have to ask, but I understand you are being courteous
to a fault. It's your module, though! I argue a lot but it *is* yours. Run
it through the armbru delinter as much as you'd like.)
> >> > +
> >> > + def connect_doc(self, doc=None):
> >> > + super().connect_doc(doc)
> >> > + doc = doc or self.doc
> >> > + if doc:
> >> > + for f in self.features:
> >> > + doc.connect_feature(f)
> >> > +
> >> > @property
> >> > def ifcond(self):
> >> > assert self._checked
> >>
> >> [...]
> >>
> >>
>
>
