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

[John Snow]

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