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

[John Snow <jsnow@redhat.com>]

On 10/8/20 5:06 AM, Markus Armbruster wrote:
> 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?
> 

As long as variants isn't re-bound in that scope, it should stick within 
that scope, yeah. (I think. Didn't test. It's my expectation.)

> 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.
> 

Yeah, understood. Well, part of it is knowing the review style of your 
reviewer too. It's been a while since we've personally swapped patches.

The actual process for much of this was: "There is an error! let me fix 
that error."

And then I do that, but I do so using idioms and patterns I'm familiar 
or comfortable with. And then post-hoc, not 100% of them were 
necessarily required, strictly, to fix the problem. Most of this series 
was written "one file at a time", and then split out post-fact into 
little per-warning changes.

I didn't even send part 1 until I finished typing the *entire* package, 
to make sure that things were as correct as possible from the very first 
commit.

I often don't really even notice that little changes aren't strictly 
necessary until they're challenged, it's usually not a conscious choice 
to try and sneak stuff in.

Still always trying to find a balance between "Easy to maintain and 
iterate" and "easy to review." Tough line for me.

>>>> +
>>>>            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