[Top][All Lists]

[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: John Snow
Subject: Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str
Date: Wed, 20 Jan 2021 10:51:40 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

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):

where @foo is a QAPISchemaFooBar, I wrote

     def visit_foo_bar(self, name, info, [curated attributes of @foo]):

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):

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
-        super().visit_module(name)
+        super().visit_module(module)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]