Code review comment for lp:~free.ekanayaka/landscape-client/method-call-defer

Revision history for this message
Duncan McGreggor (oubiwann) wrote :

This is super-cool, Free -- so glad you did this :-)

[1] 1661 tests, all passing -- nice!

[2] This isn't part of this particular change, but there's a bare exception before the deferred check:

| try:
| result = getattr(self.factory.object, method)(*args, **kwargs)
| except Exception, error:
| raise MethodCallError("Remote exception %s" % str(error))

It'd be nice if expected exceptions were handled explicitly here.

[3] You'll want to get Thomas' feedback on this suggestion... but one thing you might want to consider is actually having receive_method_call always return a deferred (with return maybeDeferred(result)). That would make the method much simpler, but it would require changes elsewhere. Regardless, probably enough work to justify being done in a separate ticket/branch ;-)

+1 for merge!

review: Approve

« Back to merge proposal