launchpadlib

Code review comment for lp:~jkakar/launchpadlib/fake-launchpad

fake-launchpad
Merge into trunk

Revision history for this message

Martin Pool (mbp) wrote on 2011-01-31:

Thanks for writing this, Jamu. This is such a big gap for anyone writing Launchpad clients. I do feel getting some thing into this gap, however imperfect, is better than nothing. It's a real shame this has been sitting here for 20 months.

Perhaps this should go into a separate project where it can evolve separately of lazr.restful/launchpadlib and be perhaps cross a lower barrier? As far as I can see it doesn't actually patch anything in launchpadlib; it only adds new classes.

The main thing I would like to see both as a reviewer and would-be user is just a README demonstrating how one could use it, and what kind of thing it's supposed to help you with.

People trying to catch up on the history of this should read https://code.edge.launchpad.net/~jkakar/launchpadlib/testing-support/+merge/14444

I'm not very familiar with the internals of the existing implementation but the code for this looks reasonable to me, at least to the point where I would not be afraid to change it. I would like to see the xml-parsing stuff split out a bit from the FakeResource, and perhaps changed to use lxml, but there's no reason that needs to be done in the first iteration.

+ def test_callable_object_return_type(self):
+ """
+ The result of a fake method is a L{FakeResource}, automatically
+ created from the object used to define the return object.
+ """
+ branches = dict(total_size="8")
+ self.launchpad.me = dict(getBranches=lambda statuses: branches)
+ branches = self.launchpad.me.getBranches([])
+ self.assertTrue(isinstance(branches, FakeResource))
+ self.assertEqual("8", branches.total_size)
+

I feel I would like this to make a bigger distinction between code that's running "behind the scenes" to set up the mock object, vs things being done on the front end of the mock object once they're set up. It seems like assigning to "me" is a behind-the-scenes setup thing and not something you can do on a real Launchpad, whereas accessing its attributes is pretending to be talking to a real one.

Maybe this is not a meaningful distinction for this library?

Thanks for writing this, Jamu.  This is such a big gap for anyone writing Launchpad clients.  I do feel getting some thing into this gap, however imperfect, is better than nothing.  It's a real shame this has been sitting here for 20 months.

Perhaps this should go into a separate project where it can evolve separately of lazr.restful/launchpadlib and be perhaps cross a lower barrier?  As far as I can see it doesn't actually patch anything in launchpadlib; it only adds new classes.

The main thing I would like to see both as a reviewer and would-be user is just a README demonstrating how one could use it, and what kind of thing it's supposed to help you with.

People trying to catch up on the history of this should read https://code.edge.launchpad.net/~jkakar/launchpadlib/testing-support/+merge/14444

I'm not very familiar with the internals of the existing implementation but the code for this looks reasonable to me, at least to the point where I would not be afraid to change it.  I would like to see the xml-parsing stuff split out a bit from the FakeResource, and perhaps changed to use lxml, but there's no reason that needs to be done in the first iteration.

+    def test_callable_object_return_type(self):
+        """
+        The result of a fake method is a L{FakeResource}, automatically
+        created from the object used to define the return object.
+        """
+        branches = dict(total_size="8")
+        self.launchpad.me = dict(getBranches=lambda statuses: branches)
+        branches = self.launchpad.me.getBranches([])
+        self.assertTrue(isinstance(branches, FakeResource))
+        self.assertEqual("8", branches.total_size)
+

I feel I would like this to make a bigger distinction between code that's running "behind the scenes" to set up the mock object, vs things being done on the front end of the mock object once they're set up.  It seems like assigning to "me" is a behind-the-scenes setup thing and not something you can do on a real Launchpad, whereas accessing its attributes is pretending to be talking to a real one.

Maybe this is not a meaningful distinction for this library?

« Back to merge proposal