Federico Simoncelli has posted comments on this change. Change subject: securable: refactor the securable implementation ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/22115/4/vdsm/storage/securable.py File vdsm/storage/securable.py: Line 28: class SecureError(RuntimeError): Line 29: pass Line 30: Line 31: Line 32: def Secured(check_method_name): > Requiring implementation of "_isSafe" does not make sense - maybe "isSafe". I like the new version better. Line 33: def wrapper(cls): Line 34: for fun, val in cls.__dict__.iteritems(): Line 35: if (not callable(val) or not getattr(val, SECURE_FIELD, True) Line 36: or fun.startswith("__") # skipping builtins Line 30: Line 31: Line 32: def Secured(check_method_name): Line 33: def wrapper(cls): Line 34: for fun, val in cls.__dict__.iteritems(): > it's not your fault, but "fun" is a misleading choice for a name of an attr Criticism on names is always well accepted, even more so if it includes a better proposal. Line 35: if (not callable(val) or not getattr(val, SECURE_FIELD, True) Line 36: or fun.startswith("__") # skipping builtins Line 37: or fun == check_method_name): Line 38: continue Line 31: Line 32: def Secured(check_method_name): Line 33: def wrapper(cls): Line 34: for fun, val in cls.__dict__.iteritems(): Line 35: if (not callable(val) or not getattr(val, SECURE_FIELD, True) > Actually using callable is even worse, because having class attributes will Actually callable on a class attribute (even a classmethod) returns False. So if that's what you were referring to, there's nothing to improve here. (You have to consider that we loop over __dict__ of the class) Line 36: or fun.startswith("__") # skipping builtins Line 37: or fun == check_method_name): Line 38: continue Line 39: setattr(cls, fun, _secure_method(val, check_method_name)) Line 47: setattr(f, SECURE_FIELD, False) Line 48: return f Line 49: Line 50: Line 51: def _secure_method(method, check_method_name): > I agree, but we can do this extra refactoring in another patch. I actually wanted to remove this. So fine by me. Line 52: @wraps(method) Line 53: def wrapper(self, *args, **kwargs): Line 54: if not getattr(self, SECURABLE_FIELD, False): Line 55: raise RuntimeError("Secured object is not a securable") -- To view, visit http://gerrit.ovirt.org/22115 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
