Re: [PATCH v2 31/38] qapi/introspect.py: add _gen_features helper

[Eduardo Habkost]

On Wed, Sep 23, 2020 at 05:43:45PM -0400, John Snow wrote:
> On 9/23/20 12:35 PM, Eduardo Habkost wrote:
> > I believe these two lines above should be removed, as suggested
> > in patch 30, but let's ignore that for now.
> > 
> 
> Yup, headed there.
> 
> > > -    if features:
> > > -        obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in 
> > > features]
> > I can't say I understand completely why moving these two lines
> > outside _make_tree() is useful, but if it makes the cleanup work
> > you did easier, I trust this is the right thing to do.  The
> > changes look correct.
> 
> The basic premise is:
> 
> Why pass information you want to add to obj['features'] to a function to
> make that assignment, when you could just perform that assignment yourself?
> 
> Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!)
> has to interrogate its arguments to make sure you gave it a dict when you
> give it a features argument.

Oh, I was not aware that obj could be not a dictionary.  In this
case, it makes lots of sense to move the magic outside the
function.

> 
> Type-wise, it's cleaner to perform this transformation when we KNOW we have
> an object than it is to defer to a more abstracted function and
> assert/downcast back to more specific types.

-- 
Eduardo