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.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/sphinx/qapidoc.py | 8 ++++----
scripts/qapi/gen.py | 16 ++++++++++------
scripts/qapi/schema.py | 4 ++--
tests/qapi-schema/test-qapi.py | 4 ++--
4 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e03abcbb959..f754f675d66 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
self._env = env
self._qapidir = qapidir
- def visit_module(self, name):
- if name is not None:
- qapifile = self._qapidir + '/' + name
+ def visit_module(self, module):
+ if module.name:
+ qapifile = self._qapidir + '/' + module.name
self._env.note_dependency(os.path.abspath(qapifile))
- super().visit_module(name)
+ super().visit_module(module)
class QAPIDocDirective(Directive):
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 3d81b90ab71..e73d3d61aac 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -31,7 +31,11 @@
guardstart,
mcgen,
)
-from .schema import QAPISchemaObjectType, QAPISchemaVisitor
+from .schema import (
+ QAPISchemaModule,
+ QAPISchemaObjectType,
+ QAPISchemaVisitor,
+)
from .source import QAPISourceInfo
@@ -304,19 +308,19 @@ def _begin_system_module(self, name: None) -> None:
def _begin_user_module(self, name: str) -> None:
pass
- def visit_module(self, name: Optional[str]) -> None:
- if name is None:
+ def visit_module(self, module: QAPISchemaModule) -> None:
+ if module.name is None:
if self._builtin_blurb:
self._add_system_module(None, self._builtin_blurb)
- self._begin_system_module(name)
+ self._begin_system_module(module.name)
else:
# The built-in module has not been created. No code may
# be generated.
self._genc = None
self._genh = None
else:
- self._add_user_module(name, self._user_blurb)
- self._begin_user_module(name)
+ self._add_user_module(module.name, self._user_blurb)
+ self._begin_user_module(module.name)
def visit_include(self, name: str, info: QAPISourceInfo) -> None:
relname = os.path.relpath(self._module_filename(self._what, name),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee4..69ba722c084 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -97,7 +97,7 @@ def visit_begin(self, schema):
def visit_end(self):
pass
- def visit_module(self, name):
+ def visit_module(self, module):
pass
def visit_needed(self, entity):
@@ -145,7 +145,7 @@ def add_entity(self, ent):
self._entity_list.append(ent)
def visit(self, visitor):
- visitor.visit_module(self.name)
+ visitor.visit_module(self)
for entity in self._entity_list:
if visitor.visit_needed(entity):
entity.visit(visitor)
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..bec1ebff3db 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -24,8 +24,8 @@
class QAPISchemaTestVisitor(QAPISchemaVisitor):
- def visit_module(self, name):
- print('module %s' % name)
+ def visit_module(self, module):
+ print('module %s' % module.name)
def visit_include(self, name, info):
print('include %s' % name)
--
2.26.2
John Snow <jsnow@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?
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> docs/sphinx/qapidoc.py | 8 ++++----
> scripts/qapi/gen.py | 16 ++++++++++------
> scripts/qapi/schema.py | 4 ++--
> tests/qapi-schema/test-qapi.py | 4 ++--
> 4 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e03abcbb959..f754f675d66 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir):
> self._env = env
> self._qapidir = qapidir
>
> - def visit_module(self, name):
> - if name is not None:
> - qapifile = self._qapidir + '/' + name
> + def visit_module(self, module):
> + if module.name:
Replacing the "is not None" test by (implicit) "is thruthy" changes
behavior for the empty string. Intentional?
I've had the "pleasure" of debugging empty strings getting interpreted
like None where they should be interpreted like any other string.
> + qapifile = self._qapidir + '/' + module.name
> self._env.note_dependency(os.path.abspath(qapifile))
> - super().visit_module(name)
> + super().visit_module(module)
>
>
[...]
On 1/20/21 6:07 AM, Markus Armbruster wrote: > John Snow <jsnow@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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 1/20/21 11:02 AM, Eric Blake wrote: > On 1/20/21 6:07 AM, Markus Armbruster wrote: >> John Snow <jsnow@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. --js
John Snow <jsnow@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 <jsnow@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!
On 1/20/21 7:07 AM, Markus Armbruster wrote: > John Snow <jsnow@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). > It's fine: you'll probably notice later I don't go the full distance and rely on both object and class methods anyway, so this isn't strictly needed right now. (It was not possible to go the full distance without heavier, more invasive changes, so...) > 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 not sure. Just taking the object would avoid a lot of duplicated interface typing, and type hints would allow editors to know what fields are available. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> docs/sphinx/qapidoc.py | 8 ++++---- >> scripts/qapi/gen.py | 16 ++++++++++------ >> scripts/qapi/schema.py | 4 ++-- >> tests/qapi-schema/test-qapi.py | 4 ++-- >> 4 files changed, 18 insertions(+), 14 deletions(-) >> >> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py >> index e03abcbb959..f754f675d66 100644 >> --- a/docs/sphinx/qapidoc.py >> +++ b/docs/sphinx/qapidoc.py >> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir): >> self._env = env >> self._qapidir = qapidir >> >> - def visit_module(self, name): >> - if name is not None: >> - qapifile = self._qapidir + '/' + name >> + def visit_module(self, module): >> + if module.name: > > Replacing the "is not None" test by (implicit) "is thruthy" changes > behavior for the empty string. Intentional? > Instinctively it was intentional, consciously it wasn't. I was worried about what "qapifile" would produce if the string happened to be empty. > I've had the "pleasure" of debugging empty strings getting interpreted > like None where they should be interpreted like any other string. > assert module.name, then? >> + qapifile = self._qapidir + '/' + module.name >> self._env.note_dependency(os.path.abspath(qapifile)) >> - super().visit_module(name) >> + super().visit_module(module) >> >> > [...] >
John Snow <jsnow@redhat.com> writes: > On 1/20/21 7:07 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: [...] >>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py >>> index e03abcbb959..f754f675d66 100644 >>> --- a/docs/sphinx/qapidoc.py >>> +++ b/docs/sphinx/qapidoc.py >>> @@ -463,11 +463,11 @@ def __init__(self, env, qapidir): >>> self._env = env >>> self._qapidir = qapidir >>> - def visit_module(self, name): >>> - if name is not None: >>> - qapifile = self._qapidir + '/' + name >>> + def visit_module(self, module): >>> + if module.name: >> Replacing the "is not None" test by (implicit) "is thruthy" changes >> behavior for the empty string. Intentional? >> > > Instinctively it was intentional, consciously it wasn't. I was worried > about what "qapifile" would produce if the string happened to be > empty. It would produce a dependency on the directory. >> I've had the "pleasure" of debugging empty strings getting interpreted >> like None where they should be interpreted like any other string. >> > > assert module.name, then? Let's avoid changing behavior in a refactoring patch. If you want an assertion to ease your worry, separate patch. >>> + qapifile = self._qapidir + '/' + module.name >>> self._env.note_dependency(os.path.abspath(qapifile)) >>> - super().visit_module(name) >>> + super().visit_module(module) >>> >> [...] >>
© 2016 - 2025 Red Hat, Inc.