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

I've gone through this branch. It's pretty complicated and I'm not prepared to do a full review yet, but I can tell you that it's good enough to build on. Go ahead and make your improvements in a branch based on this one.

I do have some suggestions and questions, from large-scale to small-scale:

1. I would like most of this code to eventually live in lazr.restfulclient, since it mostly has nothing to do with Launchpad per se. The only code that needs to live in launchpadlib is a) the credential code, b) the Launchpad WADL file itself, c) any code for Launchpad-specific index lookup operations like launchpad.people['leonardr']. This doesn't need to happen right now, but I would like it to be easy to move that code out.

2. Similarly, I would like to see most of the tests use a small, custom-designed WADL file (or several small files) rather than the Launchpad WADL file. This will make the tests easy to understand and will ensure that they don't break when the Launchpad WADL changes. It will also make it easy to put the tests in lazr.restfulclient. wadllib takes the "one WADL file per feature" approach for most of the tests added since the original release, and I think it's quite comprehensible.

3. I don't like the way you use "FakeResource" to cover the service root resource and entry resource, but not the collection resource. It seems like "FakeResource" should be a generic superclass and you should specialize. Even if the FakeServiceRootResource and FakeEntryResource are just empty subclasses of FakeResource, the code would be easier to read.

4.

+ # This is a crappy heuristic that attempts to guess the name
+ # of the key to use for index lookups.

Put the index lookups in a later branch, when you can do them properly. What is "properly"? I thought you might be able to do introspection on the CollectionWithKeyBasedLookup subclasses like PersonSet and BugSet, but I'm a little doubtful now. You might have to re-implement the _get_url_from_id implementations.

5. I don't understand why "The result of a fake method" is always a FakeResource. Your test_callable_object_return_type defines a method that returns a collection. Shouldn't it be a FakeCollectionResource? Between this and #3 I am getting the impression that FakeResource really _is_ an all-in-one resource class and that FakeCollectionResource is only used in certain circumstances. But I still don't have a good grasp of the difference.

I'll work on a more detailed review tomorrow.