From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 848BCC433DB for ; Wed, 20 Jan 2021 15:53:26 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EBFA92336D for ; Wed, 20 Jan 2021 15:53:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EBFA92336D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:45808 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1l2FnR-0005U2-1z for qemu-devel@archiver.kernel.org; Wed, 20 Jan 2021 10:53:25 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:52228) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2FmG-0004VH-Ti for qemu-devel@nongnu.org; Wed, 20 Jan 2021 10:52:12 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:37091) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1l2Fm5-000222-9d for qemu-devel@nongnu.org; Wed, 20 Jan 2021 10:52:11 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611157919; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=505LsQDDzLFOW8nq0nOoC2nN2JW6I6Bye+5gwcF8nQA=; b=JjimdlIqQY+uf3L1RLn6iIQoHHyOFj2l2EozfbCyGiiGSGFml2azoj8dnq8YAkqGJZDi8S nORIyQFBm7rG1IJ3AX0cdFkIehH5hyA8KSxrWx2i+IT0v5KHlzmi2sdoB0vjjxVRNe4HjQ GR9ZbJ7ybQpy2/q9lgoP/vOq8B8BIuo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-397-0AZT6ZI8Pwin1WTWY4Pflw-1; Wed, 20 Jan 2021 10:51:47 -0500 X-MC-Unique: 0AZT6ZI8Pwin1WTWY4Pflw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4D7E2514C for ; Wed, 20 Jan 2021 15:51:46 +0000 (UTC) Received: from [10.10.120.151] (ovpn-120-151.rdu2.redhat.com [10.10.120.151]) by smtp.corp.redhat.com (Postfix) with ESMTP id 997485D6AD; Wed, 20 Jan 2021 15:51:41 +0000 (UTC) Subject: Re: [PATCH v3 05/17] qapi: pass QAPISchemaModule to visit_module instead of str To: Markus Armbruster References: <20210119180242.1570753-1-jsnow@redhat.com> <20210119180242.1570753-6-jsnow@redhat.com> <87eeifu805.fsf@dusky.pond.sub.org> From: John Snow Message-ID: <049e81a7-c7d7-65af-129d-4d0993aff559@redhat.com> 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 MIME-Version: 1.0 In-Reply-To: <87eeifu805.fsf@dusky.pond.sub.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=216.205.24.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -30 X-Spam_score: -3.1 X-Spam_bar: --- X-Spam_report: (-3.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.167, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.094, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Cleber Rosa , qemu-devel@nongnu.org, Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 1/20/21 7:07 AM, Markus Armbruster wrote: > John Snow 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 >> --- >> 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) >> >> > [...] >