All of lore.kernel.org
 help / color / mirror / Atom feed
From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Cleber Rosa" <crosa@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	qemu-devel@nongnu.org, "Michael Roth" <mdroth@linux.vnet.ibm.com>
Subject: Re: [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module
Date: Fri, 25 Sep 2020 12:29:14 -0400	[thread overview]
Message-ID: <b9b4ded3-4e55-f987-b6ff-dc52178f7238@redhat.com> (raw)
In-Reply-To: <87wo0iroxo.fsf@dusky.pond.sub.org>

On 9/25/20 9:00 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Tue, Sep 22, 2020 at 05:00:47PM -0400, John Snow wrote:
>>> The edge case is that if the name is '', this expression returns a
>>> string instead of a bool, which violates our declared type.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/gen.py | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 9898d513ae..cb2b2655c3 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -251,7 +251,7 @@ def __init__(self, prefix, what, user_blurb, builtin_blurb, pydoc):
>>>   
>>>       @staticmethod
>>>       def _is_user_module(name):
>>> -        return name and not name.startswith('./')
>>> +        return name is not None and not name.startswith('./')
>>
>> This changes behavior if name=='', and I guess this is OK, but
>> I'm not sure.
> 
> @name is either
> 
> (1) A module pathname relative to the main module
> 
>      This is a module defined by the user.
> 
> (2) system module name, starting with './'
> 
>      This is a named system module.  We currently have two: './init' in
>      commands.py, and and './emit' in events.py.
> 
> (3) None
> 
>      This is the (nameless) system module for built-in stuff.  It
>      predates (2).  Using './builtin' would probably be better now.
> 

Yes please! This would help simplify Optional[str] to str in many 
places, and removes doubt as to what "None" might imply.

Let's queue that idea up as a cleanup for after this typing series.

> Note that (1) and (2) are disjoint: relative pathnames do not begin with
> './'.
> 
> name='' is not possible, because '' is not a valid pathname.
> 
>>                 I miss documentation on `visit_module()`,
>> `visit_include()`, and `_is_user_module()`.  I don't know what
>> `name` means here, and what is a "user module".
> 
> Valid complaints!  The code is subtle in places, without helping its
> readers along with comments or doc strings.
> 
>>
>>>   
>>>       @staticmethod
>>>       def _is_builtin_module(name):
>>> -- 
>>> 2.26.2
>>>

For now, I've done the simpler thing and wrapped the return in 
bool(...), but we will be able to do much more cleanups if we eliminate 
the possibility of "None" module names later. I'll get it all at once.

I'm adding it to my python TODO: https://gitlab.com/jsnow/qemu/-/issues/8

--js



  parent reply	other threads:[~2020-09-25 16:47 UTC|newest]

Thread overview: 190+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-22 21:00 [PATCH v2 00/38] qapi: static typing conversion, pt1 John Snow
2020-09-22 21:00 ` [PATCH v2 01/38] [DO-NOT-MERGE] qapi: add debugging tools John Snow
2020-09-22 23:43   ` Cleber Rosa
2020-09-23 16:48     ` John Snow
2020-09-22 21:00 ` [PATCH v2 02/38] qapi-gen: Separate arg-parsing from generation John Snow
2020-09-22 21:19   ` Eduardo Habkost
2020-09-23  0:00   ` Cleber Rosa
2020-09-23 17:05     ` John Snow
2020-09-24 19:24       ` Cleber Rosa
2020-09-25 11:34     ` Markus Armbruster
2020-09-25 15:37       ` John Snow
2020-09-28 11:45         ` Markus Armbruster
2020-09-22 21:00 ` [PATCH v2 03/38] qapi: move generator entrypoint into module John Snow
2020-09-22 21:23   ` Eduardo Habkost
2020-09-23 17:09     ` John Snow
2020-09-23  0:29   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 04/38] qapi: Prefer explicit relative imports John Snow
2020-09-22 21:25   ` Eduardo Habkost
2020-09-23 13:18   ` Cleber Rosa
2020-09-23 17:12     ` John Snow
2020-09-24 19:25       ` Cleber Rosa
2020-09-24 22:17         ` Beraldo Leal
2020-09-24 22:36           ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 05/38] qapi: Remove wildcard includes John Snow
2020-09-22 21:37   ` Eduardo Habkost
2020-09-23 13:27   ` Cleber Rosa
2020-09-23 17:21     ` John Snow
2020-09-24 19:27       ` Cleber Rosa
2020-09-24 20:04       ` John Snow
2020-09-22 21:00 ` [PATCH v2 06/38] qapi: delint using flake8 John Snow
2020-09-22 21:43   ` Eduardo Habkost
2020-09-23 13:37   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 07/38] qapi: add pylintrc John Snow
2020-09-22 21:54   ` Eduardo Habkost
2020-09-23 13:42   ` Cleber Rosa
2020-09-23 17:23     ` John Snow
2020-09-24 19:29       ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 08/38] qapi/common.py: Remove python compatibility workaround John Snow
2020-09-22 22:05   ` Eduardo Habkost
2020-09-23 14:37   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 09/38] qapi/common.py: Add indent manager John Snow
2020-09-22 22:22   ` Eduardo Habkost
2020-09-23 17:29     ` John Snow
2020-09-25 11:51       ` Markus Armbruster
2020-09-25 13:13         ` Eduardo Habkost
2020-09-25 13:47           ` Markus Armbruster
2020-09-25 14:42         ` John Snow
2020-09-23 14:55   ` Cleber Rosa
2020-09-23 17:30     ` John Snow
2020-09-25 11:55       ` Markus Armbruster
2020-09-25 15:41         ` John Snow
2020-09-22 21:00 ` [PATCH v2 10/38] qapi/common.py: delint with pylint John Snow
2020-09-22 21:46   ` Eduardo Habkost
2020-09-23 16:01   ` Cleber Rosa
2020-09-23 17:37     ` John Snow
2020-09-24 19:30       ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 11/38] qapi/common.py: Replace one-letter 'c' variable John Snow
2020-09-22 22:24   ` Eduardo Habkost
2020-09-23 16:15   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 12/38] qapi/common.py: check with pylint John Snow
2020-09-22 22:26   ` Eduardo Habkost
2020-09-23 16:18   ` Cleber Rosa
2020-09-23 16:30     ` John Snow
2020-09-22 21:00 ` [PATCH v2 13/38] qapi/common.py: add type hint annotations John Snow
2020-09-22 22:44   ` Eduardo Habkost
2020-09-23 17:57     ` John Snow
2020-09-23 18:19       ` Eduardo Habkost
2020-09-23 19:28   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 14/38] qapi/common.py: Convert comments into docstrings, and elaborate John Snow
2020-09-23 14:22   ` Eduardo Habkost
2020-09-23 19:38   ` Cleber Rosa
2020-09-23 21:18     ` John Snow
2020-09-25 17:02       ` Cleber Rosa
2020-09-25 17:13         ` John Snow
2020-09-22 21:00 ` [PATCH v2 15/38] qapi/common.py: move build_params into gen.py John Snow
2020-09-23 14:26   ` Eduardo Habkost
2020-09-23 20:04   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 16/38] qapi: establish mypy type-checking baseline John Snow
2020-09-23 14:29   ` Eduardo Habkost
2020-09-23 20:11   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 17/38] qapi/events.py: add type hint annotations John Snow
2020-09-23 14:31   ` Eduardo Habkost
2020-09-23 20:18   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 18/38] qapi/events.py: Move comments into docstrings John Snow
2020-09-23 14:48   ` Eduardo Habkost
2020-09-23 18:21     ` John Snow
2020-09-25 12:19     ` Markus Armbruster
2020-09-25 15:55       ` John Snow
2020-09-28 11:49         ` Markus Armbruster
2020-09-28 15:04           ` John Snow
2020-09-23 20:19   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 19/38] qapi/commands.py: Don't re-bind to variable of different type John Snow
2020-09-23 14:48   ` Eduardo Habkost
2020-09-23 20:21   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 20/38] qapi/commands.py: add notational type hints John Snow
2020-09-23 14:50   ` Eduardo Habkost
2020-09-23 18:23     ` John Snow
2020-09-23 22:17   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 21/38] qapi/commands.py: enable checking with mypy John Snow
2020-09-23 14:51   ` Eduardo Habkost
2020-09-23 22:21   ` Cleber Rosa
2020-09-23 23:50     ` John Snow
2020-09-22 21:00 ` [PATCH v2 22/38] qapi/source.py: add type hint annotations John Snow
2020-09-23 14:52   ` Eduardo Habkost
2020-09-23 22:36   ` Cleber Rosa
2020-09-23 23:55     ` John Snow
2020-09-25 12:22       ` Markus Armbruster
2020-09-25 16:20         ` John Snow
2020-09-25 17:05       ` Cleber Rosa
2020-09-25 17:20         ` John Snow
2020-09-22 21:00 ` [PATCH v2 23/38] qapi/source.py: delint with pylint John Snow
2020-09-23 14:59   ` Eduardo Habkost
2020-09-23 22:41   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 24/38] qapi/gen.py: Fix edge-case of _is_user_module John Snow
2020-09-23 15:17   ` Eduardo Habkost
2020-09-23 18:29     ` John Snow
2020-09-23 18:33       ` Eduardo Habkost
2020-09-23 23:10         ` Cleber Rosa
2020-09-23 23:13           ` Cleber Rosa
2020-09-25 13:00     ` Markus Armbruster
2020-09-25 15:15       ` Eduardo Habkost
2020-09-25 15:50         ` Eduardo Habkost
2020-09-28 12:04           ` Markus Armbruster
2020-09-25 16:29       ` John Snow [this message]
2020-09-23 23:08   ` Cleber Rosa
2020-09-23 23:13     ` Cleber Rosa
2020-09-23 23:57     ` John Snow
2020-09-22 21:00 ` [PATCH v2 25/38] qapi/gen.py: add type hint annotations John Snow
2020-09-23 15:18   ` Eduardo Habkost
2020-09-23 23:51   ` Cleber Rosa
2020-09-24  0:29     ` John Snow
2020-09-24  1:29       ` Eduardo Habkost
2020-09-22 21:00 ` [PATCH v2 26/38] qapi/gen.py: Enable checking with mypy John Snow
2020-09-23 15:18   ` Eduardo Habkost
2020-09-23 23:59   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 27/38] qapi/gen.py: Remove unused parameter John Snow
2020-09-23 15:19   ` Eduardo Habkost
2020-09-24  0:00   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 28/38] qapi/gen.py: update write() to be more idiomatic John Snow
2020-09-23 15:26   ` Eduardo Habkost
2020-09-23 18:37     ` John Snow
2020-09-24 15:59       ` Cleber Rosa
2020-09-25 13:15         ` Markus Armbruster
2020-09-25 13:24           ` Daniel P. Berrangé
2020-09-25 13:34             ` Eric Blake
2020-09-25 13:52             ` Markus Armbruster
2020-09-25 15:47               ` Eric Blake
2020-09-28 12:09                 ` Markus Armbruster
2020-09-28 14:08                   ` John Snow
2020-09-25 13:26           ` Eduardo Habkost
2020-09-25 16:33             ` John Snow
2020-09-24 16:01   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 29/38] qapi/gen.py: delint with pylint John Snow
2020-09-23 15:44   ` Eduardo Habkost
2020-09-23 18:38     ` John Snow
2020-09-24 18:44   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 30/38] qapi/introspect.py: Add a typed 'extra' structure John Snow
2020-09-23 16:13   ` Eduardo Habkost
2020-09-23 21:34     ` John Snow
2020-09-22 21:00 ` [PATCH v2 31/38] qapi/introspect.py: add _gen_features helper John Snow
2020-09-23 16:35   ` Eduardo Habkost
2020-09-23 21:43     ` John Snow
2020-09-23 21:54       ` Eduardo Habkost
2020-09-22 21:00 ` [PATCH v2 32/38] qapi/introspect.py: create a typed 'Node' data structure John Snow
2020-09-23 18:41   ` Eduardo Habkost
2020-09-23 21:48     ` John Snow
2020-09-23 22:44     ` John Snow
2020-09-22 21:00 ` [PATCH v2 33/38] qapi/introspect.py: add type hint annotations John Snow
2020-09-22 21:00 ` [PATCH v2 34/38] qapi/types.py: " John Snow
2020-09-23 19:11   ` Eduardo Habkost
2020-09-24 18:50   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 35/38] qapi/types.py: remove one-letter variables John Snow
2020-09-23 19:14   ` Eduardo Habkost
2020-09-23 22:11     ` John Snow
2020-09-24 20:54       ` Eduardo Habkost
2020-09-24 18:53   ` Cleber Rosa
2020-09-22 21:00 ` [PATCH v2 36/38] qapi/visit.py: assert tag_member contains a QAPISchemaEnumType John Snow
2020-09-23 19:15   ` Eduardo Habkost
2020-09-23 22:13     ` John Snow
2020-09-24 19:12       ` Cleber Rosa
2020-09-24 19:10   ` Cleber Rosa
2020-09-24 19:36     ` John Snow
2020-09-24 23:52       ` Cleber Rosa
2020-09-22 21:01 ` [PATCH v2 37/38] qapi/visit.py: remove unused parameters from gen_visit_object John Snow
2020-09-23 19:16   ` Eduardo Habkost
2020-09-24 19:13   ` Cleber Rosa
2020-09-22 21:01 ` [PATCH v2 38/38] qapi/visit.py: add type hint annotations John Snow
2020-09-23 19:17   ` Eduardo Habkost
2020-09-24 19:15   ` Cleber Rosa
2020-09-24 20:37 ` [PATCH v2 00/38] qapi: static typing conversion, pt1 John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b9b4ded3-4e55-f987-b6ff-dc52178f7238@redhat.com \
    --to=jsnow@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=mdroth@linux.vnet.ibm.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.