Re: [PATCH v5 34/36] qapi/visit.py: assert tag_member contains a QAPISchemaEnumType

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 10/7/20 8:43 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> This is true by design, but not presently able to be expressed in the
>>> type system. An assertion helps mypy understand our constraints.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Reviewed-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>   scripts/qapi/visit.py | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>>> index 14f30c228b7..4f11fd325b8 100644
>>> --- a/scripts/qapi/visit.py
>>> +++ b/scripts/qapi/visit.py
>>> @@ -22,7 +22,7 @@
>>>       mcgen,
>>>   )
>>>   from .gen import QAPISchemaModularCVisitor, ifcontext
>>> -from .schema import QAPISchemaObjectType
>>> +from .schema import QAPISchemaEnumType, QAPISchemaObjectType
>>>     
>>>   def gen_visit_decl(name, scalar=False):
>>> @@ -84,15 +84,17 @@ def gen_visit_object_members(name, base, members, 
>>> variants):
>>>           ret += gen_endif(memb.ifcond)
>>>         if variants:
>>> +        tag_member = variants.tag_member
>>> +        assert isinstance(tag_member.type, QAPISchemaEnumType)
>> I'm curious: do you need the local variable to make the assertion
>> stick?
>> 
>
> No, but it only sticks to the binding and not the
> data. i.e. assertions to downcast work on the *name*.

Would it stick to variants.tag_member, too?

I'm asking to learn, not to find a reason to make you change your patch.

> (This comes up somewhere in the schema.py patches where I make a
> change that looks completely pointless, but it makes mypy happy.)
>
> I could have left it alone. I just saw a lot of repeated multi-dots
> and habitually created a temporary local for the purpose.

Matter of taste.  Long chains of dots make the code hard to read because
they are so long.  Temporary variable make it hard to read because you
have to remember what they mean.  Tradeoff.  I come up with cases I find
hard to decide all too often.

In case the local variable isn't needed for mypy: when you throw in
something that isn't needed for the patch's stated purpose, it's best to
mention it in the commit message, because not mentioning it is a review
comment magnet :)

Put yourself in the reviewers shoes.  Your lovingly crafted commit
message puts him into a positive mood.  He nods along while reading your
obvious patch at a good pace.  And then he runs smack into the
unexpected unrelated part, and stops: oh, what's going on here?  Back up
some and read more slowly to make sure I understand.

>>> +
>>>           ret += mcgen('''
>>>       switch (obj->%(c_name)s) {
>>>   ''',
>>> -                     c_name=c_name(variants.tag_member.name))
>>> +                     c_name=c_name(tag_member.name))
>>>             for var in variants.variants:
>>> -            case_str = c_enum_const(variants.tag_member.type.name,
>>> -                                    var.name,
>>> -                                    variants.tag_member.type.prefix)
>>> +            case_str = c_enum_const(tag_member.type.name, var.name,
>>> +                                    tag_member.type.prefix)
>>>               ret += gen_if(var.ifcond)
>>>               if var.type.name == 'q_empty':
>>>                   # valid variant and nothing to do