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

[John Snow]

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