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