Michael Pasternak has posted comments on this change. Change subject: sdk: Save attributes to decorated object ......................................................................
Patch Set 3: Code-Review-1 (5 comments) http://gerrit.ovirt.org/#/c/30653/3/Makefile File Makefile: Line 1: all: rpm Line 2: Line 3: rpmrelease:=0.4 i guess, this is not a part of this change Line 4: rpmversion=3.6.0.0 Line 5: RPMTOP=$(shell bash -c "pwd -P")/rpmtop Line 6: SPEC=ovirt-engine-sdk-python.spec Line 7: http://gerrit.ovirt.org/#/c/30653/3/src/ovirtsdk/infrastructure/common.py File src/ovirtsdk/infrastructure/common.py: Line 34: attribute named "superclass". This name is required for backwards Line 35: compatibility with the CLI.""" Line 36: Line 37: def __init__(self, context): Line 38: self.__dict__[CONTEXT_ATTR] = context juan, you shouldn't expose 'context' as plain attribute, by this you loosing encapsulation of it (up until now it was hidden), user should not be able accessing it as he potentially could override proxy, etc., notice in __init__ in initialized as private variable. Line 39: Line 40: def __getattr__(self, name): Line 41: decorated = self.__dict__.get(DECORATED_ATTR) Line 42: if name == DECORATED_ATTR: Line 41: decorated = self.__dict__.get(DECORATED_ATTR) Line 42: if name == DECORATED_ATTR: Line 43: return decorated Line 44: if name == CONTEXT_ATTR: Line 45: return self.__dict__.get(CONTEXT_ATTR) please avoid doing this (based on argument above) Line 46: if decorated is not None: Line 47: return getattr(decorated, name) Line 48: if name in self.__dict__: Line 49: return self.__dict__[name] Line 43: return decorated Line 44: if name == CONTEXT_ATTR: Line 45: return self.__dict__.get(CONTEXT_ATTR) Line 46: if decorated is not None: Line 47: return getattr(decorated, name) i guess this if clouse should come after the one bellow [1], otherwise you not changing existent __getattr__ (other than returning 'context') [1] if name in self.__dict__ Line 48: if name in self.__dict__: Line 49: return self.__dict__[name] Line 50: raise AttributeError("'%s' broker object has no attribute '%s'" % (type(self).__name__, name)) Line 51: Line 58: decorated = self.__dict__.get(DECORATED_ATTR) Line 59: if decorated is not None: Line 60: setattr(decorated, name, value) Line 61: else: Line 62: self.__dict__[name] = value Hi Juan, 1. using dict based assignments is not good in __setattr__ context, cause if user has a typo in attribute name, you'll silently swallow it by creating new attribute in the dict witch eventually will not be serialized and send to client (i.e it will be ignored), i'd stay with standard __setattr__ procedure which already has treatment for this. 2. there is no properties to update in base class, only super-class is updatebale, better throwing AttributeError here, otherwise you will create new attributes here while intention was changing superclass attributes. thanks. Line 63: Line 64: def __eq__(self, other): Line 65: return Comparator.compare(self, other) Line 66: -- To view, visit http://gerrit.ovirt.org/30653 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a4c5e08f844b250b585508c962fc0c38f9aad98 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine-sdk Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Michael Pasternak <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
