[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str |
Date: |
Wed, 20 Jan 2021 13:07:22 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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)
>
>
[...]
- [PATCH v3 00/17] qapi: static typing conversion, pt1.5, John Snow, 2021/01/19
- [PATCH v3 03/17] qapi/main: handle theoretical None-return from re.match(), John Snow, 2021/01/19
- [PATCH v3 01/17] qapi/commands: assert arg_type is not None, John Snow, 2021/01/19
- [PATCH v3 06/17] qapi: centralize is_[user|system|builtin]_module methods, John Snow, 2021/01/19
- [PATCH v3 08/17] qapi: use explicitly internal module names, John Snow, 2021/01/19
- [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str, John Snow, 2021/01/19
[PATCH v3 07/17] qapi/gen: Replace ._begin_system_module(), John Snow, 2021/01/19
[PATCH v3 11/17] qapi: centralize the built-in module name definition, John Snow, 2021/01/19
[PATCH v3 04/17] qapi/gen: inline _wrap_ifcond into end_if(), John Snow, 2021/01/19
[PATCH v3 12/17] qapi/gen: write _genc/_genh access shims, John Snow, 2021/01/19
[PATCH v3 16/17] qapi: type 'info' as Optional[QAPISourceInfo], John Snow, 2021/01/19