On Sat, Jan 21, 2012 at 12:47:08AM -0800, Danek Duvall wrote: > Edward Pilatowicz wrote: > > > i did this because if you have an Action object and you try doing: > > > > if src != None: > > > > then you'll get an exception: > > > > ---8<--- > > File > > "/export/ws/pkg.pkgplan_cleanup/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/actions/generic.py", > > line 431, in __ne__ > > if self.name == other.name and \ > > AttributeError: 'NoneType' object has no attribute 'name' > > ---8<--- > > > > because the __ne__() function for action objects assumes your comparing > > the current action against another action (and not some other type of > > object). > > > > so i changed all getstate() and setstate() code to use: > > > > type(XXX) != type(None) > > > > because that makes the comparison independent of any __ne__() > > implementation in the class your comparing against (and for consistency, > > i did this for all the "None" comparisons in the *state()). if you'd > > like me to do this some other way then just let me know what's > > preferable. > > Why not simply have __ne__() in the generic action class do the comparison > against None, and return True if so? Probably want an equivalent test in > __eq__() as well. Or you could test with isinstance(), as in __cmp__(), > though in that case you'd want to return True or False instead of > NotImplemented. All of this should be performance tested, regardless. > > And if you're comparing against None, then you're better off comparing > using the "is" operator, which would sidestep this issue entirely, as well > as be more performant. >
i didn't modify __ne__() before because i didn't know if the current behavior was intentional (i didn't know if perhaps there was a convention to never compare actions to other object types and perhaps this was a mechanism to enforce that). that said, i think i'll just go with the "if XXX is not None" option since that doesn't risk a perf impact. wrt performance, i have tested an install -n (upgrading from s11 fcs to s11u1b7) and verified that there is no performance regression. > Besides (I'm just looking at your second webrev's pkgplan code now), why > are you comparing against type(None) instead of isinstance(basestring), > since the only valid input to fromstr() is a string? > in setstate() we're taking json data as input. if the data is valid will either be None or a string, so i'm checking against None because if it's not None and it's not a string then we've got invalid data and we should throw an exception. ed _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
