All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Cleber Rosa <crosa@redhat.com>,
	qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v2 04/16] qapi/expr.py: Add assertion for union type 'check_dict'
Date: Wed, 18 Nov 2020 16:30:00 +0100	[thread overview]
Message-ID: <87blfuisxz.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201026213637.47087-5-jsnow@redhat.com> (John Snow's message of "Mon, 26 Oct 2020 17:36:25 -0400")

John Snow <jsnow@redhat.com> writes:

> mypy isn't fond of allowing you to check for bool membership in a
> collection of str elements. Guard this lookup for precisely when we were
> given a name.

Spoilsport.

Peeking at the patch... aha, it's about check_type()'s parameter
@allow_dict.

@allow_dict tells us whether an anonymous type is allowed, and also
whether its member names may violate the naming rules.

* a str: allow anonymous type, waive member naming rules if @allow_dict
  is in .name_case_whitelist.

  Used for checking struct's 'data' and union's 'base'.

* True: allow anonymous type, enforce member naming rules

  Used for checking 'data' of commands and events.  Waiving the naming
  rules is simply not implemented there.

* False (default): do not allow anonymous type

Perhaps the "is in .name_case_whitelist" check should be lifted into the
two callers that pass a str.  We could then turn the parameter into an
enum.  Meh.  Perhaps a separate @permit_upper parameter, only valid with
allow_dict=True.  Meh again.

Splitting check_type() into multiple functions feels more promising.
Not now.

> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> ---
>  scripts/qapi/expr.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index f7c7f91326ef..2c4c341d5243 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -173,7 +173,9 @@ def check_type(value, info, source,
>          raise QAPISemError(info,
>                             "%s should be an object or type name" % source)
>  
> -    permit_upper = allow_dict in info.pragma.name_case_whitelist
> +    permit_upper = False
> +    if isinstance(allow_dict, str):
> +        permit_upper = allow_dict in info.pragma.name_case_whitelist

Slightly more compact:

       permit_upper = (isinstance(allow_dict, str)
                       and allow_dict in info.pragma.name_case_whitelist)

Matter of taste.

>  
>      # value is a dictionary, check that each member is okay
>      for (key, arg) in value.items():



  reply	other threads:[~2020-11-18 15:31 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-26 21:36 [PATCH v2 00/16] qapi: static typing conversion, pt3 John Snow
2020-10-26 21:36 ` [PATCH v2 01/16] qapi/expr.py: Remove 'info' argument from nested check_if_str John Snow
2020-10-26 21:36 ` [PATCH v2 02/16] qapi/expr.py: Check for dict instead of OrderedDict John Snow
2020-11-17 16:30   ` Markus Armbruster
2020-11-17 18:08     ` Eduardo Habkost
2020-11-17 19:48       ` John Snow
2020-10-26 21:36 ` [PATCH v2 03/16] qapi/expr.py: constrain incoming expression types John Snow
2020-11-18 14:58   ` Markus Armbruster
2020-10-26 21:36 ` [PATCH v2 04/16] qapi/expr.py: Add assertion for union type 'check_dict' John Snow
2020-11-18 15:30   ` Markus Armbruster [this message]
2020-10-26 21:36 ` [PATCH v2 05/16] qapi/expr.py: move string check upwards in check_type John Snow
2020-10-26 21:36 ` [PATCH v2 06/16] qapi/expr.py: Check type of 'data' member John Snow
2020-11-18 15:36   ` Markus Armbruster
2020-10-26 21:36 ` [PATCH v2 07/16] qapi/expr.py: Add casts in a few select cases John Snow
2020-11-18 15:41   ` Markus Armbruster
2020-10-26 21:36 ` [PATCH v2 08/16] qapi/expr.py: add type hint annotations John Snow
2020-10-26 21:36 ` [PATCH v2 09/16] qapi/expr.py: rewrite check_if John Snow
2020-10-26 21:36 ` [PATCH v2 10/16] qapi/expr.py: Remove single-letter variable John Snow
2020-10-26 21:36 ` [PATCH v2 11/16] qapi/expr.py: enable pylint checks John Snow
2020-10-26 21:36 ` [PATCH v2 12/16] qapi/expr.py: Add docstrings John Snow
2020-10-26 21:36 ` [PATCH v2 13/16] qapi/expr.py: Modify check_keys to accept any Iterable John Snow
2020-10-26 21:36 ` [PATCH v2 14/16] qapi/expr.py: Use tuples instead of lists for static data John Snow
2020-10-26 21:36 ` [PATCH v2 15/16] qapi/expr.py: move related checks inside check_xxx functions John Snow
2020-10-26 21:36 ` [PATCH v2 16/16] qapi/expr.py: Use an expression checker dispatch table John Snow
2020-11-04  0:41 ` [PATCH v2 00/16] qapi: static typing conversion, pt3 John Snow
2020-11-04 14:53   ` Marc-André Lureau

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=87blfuisxz.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=crosa@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jsnow@redhat.com \
    --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.