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
