qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx


From: John Snow
Subject: Re: [PATCH v3 15/16] qapi/expr.py: move related checks inside check_xxx functions
Date: Thu, 25 Mar 2021 01:17:38 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

On 2/25/21 10:28 AM, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:

There's not a big obvious difference between the types of checks that
happen in the main function versus the kind that happen in the
functions. Now they're in one place for each of the main types.

As part of the move, spell out the required and optional keywords so
they're obvious at a glance. Use tuples instead of lists for immutable
data, too.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>

No objection to changing read-only lists to tuples (applies to previous
patch, too).

No objection to turning positional into keyword arguments where that
improves clarity.

I have doubts on the code motion.  Yes, the checks for each type are now
together.  On the other hand, the check_keys() are now separate.  I can
no longer see all the keys at a glance.


I guess it depends on where you wanted to see them; I thought it was strange that in check_foobar I couldn't see what foobar's valid keys were without scrolling back to the bottom of the file.

Needing to see all the keys for the disparate forms together was not a case I ran into, but you can always drop this patch for now if you'd like. I had some more adventurous patches that keeps pushing in this direction, but I don't know if it's really important. My appetite in this area has waned since November.

--js




reply via email to

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