Comment 5 for bug 499018

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Looks good to me +1. I dig the generic method call api over amp. diffs below are my suggested changes against the branch.

[1] doc string typo.
--- landscape/lib/amp.py 2010-01-18 08:24:51 +0000
+++ landscape/lib/amp.py 2010-01-19 17:57:36 +0000
@@ -82,7 +82,7 @@

         @param method: The name of the remote method to invoke.
         @param args: The positional arguments to pass to the remote method.
- @param args: The keyword arguments to pass to the remote method.
+ @param kwargs: The keyword arguments to pass to the remote method.
         """
         return self.callRemote(MethodCall,
                                method=method, args=args, kwargs=kwargs)

[2] There's another argument copy. I removed it and the tests run fine.

@@ -147,8 +147,8 @@

         def send_method_call(*args, **kwargs):
             result = self._protocol.send_method_call(method=method,
- args=args[:],
- kwargs=kwargs.copy())
+ args=args,
+ kwargs=kwargs)
             return result.addCallback(lambda response: response["result"])

[3] It would be nice but certainly not required to see a unit test with an exception in the remote method implementation, being caught by the client. Mostly as an example. Triggering it by hand i get an UnknownAmpRemoteError, which i guess is to be expected.