John Snow <js...@redhat.com> writes: > On 1/20/21 11:02 AM, Eric Blake wrote: >> On 1/20/21 6:07 AM, Markus Armbruster wrote: >>> John Snow <js...@redhat.com> writes: >>> >>>> Modify visit_module to pass the module itself instead of just its >>>> name. This allows for future patches to centralize some >>>> module-interrogation behavior within the QAPISchemaModule class itself, >>>> cutting down on duplication between gen.py and schema.py. >>> >>> We've been tempted to make similar changes before (don't worry, I'm not >>> building a case for "no" here). >>> >>> When I wrote the initial version of QAPISchemaVisitor (commit 3f7dc21be, >>> 2015), I aimed for a loose coupling of backends and the internal >>> representation. Instead of >>> >>> def visit_foo(self, foo): >>> pass >>> >>> where @foo is a QAPISchemaFooBar, I wrote >>> >>> def visit_foo_bar(self, name, info, [curated attributes of @foo]): >>> pass >>> >>> In theory, this is nice: the information exposed to the backends is >>> obvious, and the backends can't accidentally mutate @foo. >>> >>> In practice, it kind of failed right then and there: >>> >>> def visit_object_type(self, name, info, base, members, variants): >>> pass >>> >>> We avoid passing the QAPISchemaObjectType (loose coupling, cool!), only >>> to pass member information as List[QAPISchemaObjectTypeMember]. >>> >>> Morever, passing "curated atttibutes" has led to visit_commands() taking >>> a dozen arguments. Meh. >>> >>> This had made Eric and me wonder whether we should write off the >>> decoupling idea as misguided, and just pass the object instead of >>> "curated attributes", always. Thoughts? >> I'm open to the idea of passing just the larger object instead of >> the >> curated list of relevant attributes. It's a bit more coupling, but I >> don't see any of our qapi code being reused outside its current scope >> where the extra coupling will bite us. But I'm not volunteering for the >> refactoring work, because I'm not an expert on python typing hints. If >> consolidating parameters into the larger object makes for fewer >> parameters and easier typing hints, I'm assuming the work can be done as >> part of static typing; but if leaving things as they currently are is >> manageable, that's also fine by me. >> > > Yeah, it can definitely be left as-is for now. I've already gone > through all the effort of typing out all of the interfaces, so it's > not really a huge ordeal to just leave it as-is. > > Passing the objects might be nicer for the future, though, as routing > new information or features will involve less churn. (And the > signatures will be a lot smaller.) > > I suppose it does open us up to callers mutating the schema in the > visitors, but they could already do that for the reasons that Markus > points out. It's possible that the visitor dispatch could be modified > to make deep copies of schema objects, but that adds overhead. > > I can just revert this change for now and leave the topic for another day.
Works for me. Thanks!