On Wed, Sep 14, 2011 at 12:51 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> I see the bikeshedding has already been done.  Good—you won't be needing much from me then.  Thanks for putting your weight behind this project!
>

No worries.  A lot of your input here has been very useful.

FWIW, all I've done for this has been to take Jamu's branch and reply
to other people's review comments (now yours).

> My notes are essentially about superficialities.  See below.  What they mostly boil down to is that you seem to be missing an abstraction layer to translate parsed WADL into meaningful information.  In practice it looks like a matter of extracting lots of little helpers to separate the parsing nitty-gritty from the real meaning of the Fake* code.

Yeah. Looking at this code, I think it would be worthwhile to extract
something that's very much focused on WADL interpretation. (e.g. a
ResourceType class). However, as I declared in the beginning, that's
out of scope. I just want to merge Jamu's useful code to prove that
useful code can be merged! When I have confidence that this is even
possible, I'll merrily refactor away.

> === added file 'src/launchpadlib/testing/launchpad.py'
>
> —8<—
>
> +class FakeResource(object):
> +    """
> +    Fake resource validates and represents sample data set on L{FakeLaunchpad}
> +    instances.
>
> Found this a bit hard to read.  Maybe leave out the “Fake resource” at the beginning?  The “set” could be a verb or a noun, so maybe just say “Valides and represents sample data on a L{FakeLaunchpad}”?  Has the added benefit of fitting on a single line.
>

Changed.

>
> +    @param application: A C{waddlib.application.Application} instance.
> +    @param resource_type: A C{wadllib.application.ResourceType} instance for
> +        this resource.
> +    @param values: Optionally, a dict representing attribute key/value pairs
> +        for this resource.
>
> You're using a bunch of documentation conventions I'm not familiar with.  Do constructor parameters normally go into the class docstring then?
>

They normally do. This was how I found them. Have moved into constructor.

>
> +    def __setattr__(self, name, value):
> +        """Set sample data.
> +
> +        @param name: The name of the attribute.
> +        @param values: A dict representing an object matching a resource
> +            defined in Launchpad's WADL definition.
>
> Which is it—“value” or “values”?  The code suggests that it can be a dict or… an atomic value I suppose:
>

Have deleted the param documentation: setattr doesn't need it.
Explained how value works.

>
> +        if isinstance(value, dict):
> +            self._children[name] = self._create_child_resource(name, value)
> +        else:
> +            values = {}
> +            if self._values is not None:
> +                values.update(self._values)
> +            values[name] = value
> +            self._check_resource_type(self._resource_type, values)
> +            self.__dict__["_values"] = values
>
> A comment might be helpful.  Does it make sense to run a full modified copy of self._values through _check_resource_type?  It checks each item individually anyway, right?

It's actually a full-blown partial type check. The updated dict is
checked to see whether it is type conformant. Comment added.

>
>
> +    def __getattr__(self, name):
> +        """Get sample data.
> +
> +        @param name: The name of the attribute.
> +        """
> +        result = self._children.get(name)
>
> What kind of items can I expect to find in _children?
>

I've added documentation in this regard to the class docstring.

>
> +        if not result:
>
>  What exactly are you testing for?  Can result be, say, an integer that might be zero, or a string that might be empty?
>

Have changed this to a marker-based get.

>
> +            if isinstance(self._values, dict):
> +                result = self._values.get(name)
> +                if callable(result):
> +                    return self._wrap_method(name, result)
>
> What does it mean for self._values not to be a dict?  One case we've seen is that it might be None… are there other possibilities?  If no, then please harmonize the test and maybe wrap it in a meaningfully-named method.
>

It's a long way of saying "not None". I've changed the logic so that
values is *always* set to a dict that defaults to {}. This has
simplified the logic here and in other places.

>
> +    def _create_child_resource(self, name, values):
> +        """
> +        Ensure that C{values} is a valid object for the C{name} attribute and
> +        return a resource object to represent it as API data.
> +
> +        @param name: The name of the attribute to check the C{values} object
> +            against.
> +        @param values: A dict with key/value pairs representing attributes and
> +            methods of an object matching the C{name} resource's definition.
> +        @return: A L{FakeEntry} for an ordinary resource or a
> +            L{FakeCollection} for a resource that represents a collection.
> +        @raises IntegrityError: Raised if C{name} isn't a valid attribute for
> +            this resource or if C{values} isn't a valid object for the C{name}
> +            attribute.
> +        """
> +        root_resource = self._application.get_resource_by_path("")
> +        is_link = False
> +        param = root_resource.get_parameter(name + "_collection_link",
> +                                            JSON_MEDIA_TYPE)
> +        if param is None:
> +            is_link = True
> +            param = root_resource.get_parameter(name + "_link", JSON_MEDIA_TYPE)
> +        if param is not None:
> +            resource_type = self._get_resource_type(param)
> +            if is_link:
> +                self._check_resource_type(resource_type, values)
> +                return FakeEntry(self._application, resource_type, values)
> +            else:
> +                name, child_resource_type = (
> +                    self._check_collection_type(resource_type, values))
> +                return FakeCollection(self._application, resource_type, values,
> +                                      name, child_resource_type)
> +        else:
> +            raise IntegrityError("%s isn't a valid property." % (name,))
>
> If you switch the “if” and “else” clauses around here, the error becomes an obvious escape path and you won't need the “else” block.  Saves you a negation in the condition and an indentation.
>

Good call. Done.

>
> +    def _check_attribute(self, resource_type, name, value):
> +        """
> +        Ensure that C{value} is a valid C{name} attribute on C{resource_type}.
> +
> +        @param resource_type: The resource type to check the attribute
> +            against.
> +        @param name: The name of the attribute.
> +        @param value: The value to check.
> +        """
> +        get_methods = [method for method in resource_type.tag.getchildren()
> +                       if method.get("name") == "GET"]
> +        for response in get_methods[0].getchildren():
>
> It looks like you're looking for a single entry.  Maybe assign to [get_method] instead, and then use get_method instead of get_methods[0]?
>

This has been rather much rejiggered.

_check_method has become _get_method, and does the lookup described
here. The loop (outer & inner) part here has been turned into
_find_representation_id, which also has the _lookup_xml_id call. This
collapses the whole method down to:

        xml_id = self._find_representation_id(resource_type, 'get')
        self._check_attribute_representation(xml_id, name, value)

Note that there are in fact multiple methods that match the search, so
[get_method] = [...] doesn't work. The search was also theoretically
(but not practically) incorrect, looking for the *first* GET method
rather than the canonical GET method.
...
>
> A “break” like this can easily lead to maintenance problems—this code may become clearer if you can wrap the inner loop or a portion of it in a separate method.

See above.

>
>
> +    def _create_resource(self, resource_type, name, result):
> +        """Create a new L{FakeResource} for C{resource_type} method call result.
> +
> +        @param resource_type: The resource type of the method.
> +        @param name: The name of the method on C{resource_type}.
> +        @param result: The result of calling the method.
> +        @raises IntegrityError: Raised if C{result} is an invalid return value
> +            for the method.
> +        @return: A L{FakeResource} for C{result}.
> +        """
> +        resource_name = resource_type.tag.get("id")
> +        if resource_name == name:
> +            name = "get"
> +        xml_id = "%s-%s" % (resource_name, name)
> +        [get_method] = [method for method in resource_type.tag.getchildren()
> +                        if method.get("id") == xml_id]
>
> My eyes are starting to glaze over.  A lot of this code hard to read; part of that is tough and dense subject matter, part is too much text crammed against the right margin, and part is because of little patterns like this one.  If you had that extra little layer of abstraction, I wouldn't need to “decompile” all these menial fragments to get to the meaning of the code.  This one seems to be a “find child tag by id.”  Could have gone into a helpfully-named function!
>

As mentioned in the cover letter and the intro to this reply, I'm not
going to go as far as either of us would like. However, I think there
are enough new functions post this review to make it much clearer.

>
> +        [response] = [element for element in get_method.getchildren()
> +                      if element.tag.split("}")[-1] == "response"]
>
> There's more unexplained low-level logic here.  Move it somewhere out of the way and give it a meaningful name!
>

This doesn't appear to be relevant. Collapsed into _find_representation_id.

>
> +        for representation in response.getchildren():
> +            representation_url = representation.get("href")
> +            if representation_url is not None and isinstance(result, dict):
> +                xml_id = self._application.lookup_xml_id(representation_url)

Collapsed into _find_representation_id.

> +                if xml_id.endswith("-full"):
> +                    xml_id = xml_id[:-5]
>
> This seems to mean “if there's a -full suffix on xml_id, strip it off.”  Replacing it with a one-liner function call would remove the clutter and the cyclomatic complexity from view.
>

Changed to use rstrip.

>
> +                if xml_id in self._application.resource_types:
> +                    result_resource_type = (
> +                        self._application.resource_types[xml_id])
> +                else:
> +                    result_resource_type = (
> +                        self._application.resource_types[xml_id + "-resource"])
> +                self._check_resource_type(result_resource_type, result)
>
> This looks like more or less the opposite of the “strip a suffix” pattern we saw earlier.  I think shorter could be clearer:
>
>    if xml_id not in self._application.resource_types:
>        # Mysterious reason explained here.
>        xml_id += "-resource"
>    result_resource_type = self._application.resource_types[xml_id]
>

Sadly, I can't explain the mysterious reason. It has to do with the
conventions in lazr.restful, but they are beyond me at the moment.

>
> +    def _get_child_resource_type(self, resource_type):
> +        """
> +        Ensure that C{value} is a valid C{name} attribute on C{resource_type}.
>
> The docstring does not appear to match the name.  Are you getting or checking?
>

Both. I've changed the docstring to match the behaviour and type signature.

>
> +        @param resource_type: The resource type to check the attribute
> +            against.
> +        @param name: The name of the attribute.
> +        @param value: The value to check.
> +        """
> +        representation_url = None
> +        get_methods = [method for method in resource_type.tag.getchildren()
> +                       if method.get("name") == "GET"]
>
> I've seen this fragment before.  Please extract it.
>

Done.

> +        for response in get_methods[0].getchildren():
> +            for representation in response.getchildren():
> +                representation_url = representation.get("href")
> +                if representation_url is not None:
> +                    break
>
> Is this meant to be a “find first child with an href”?  Please extract.
>

_find_representation_id.

>
> +        xml_id = self._application.lookup_xml_id(representation_url)
> +        representation_definition = (
> +            self._application.representation_definitions[xml_id])
> +
> +        [entry_links] = [
> +            param for param in representation_definition.tag.getchildren()
> +            if param.get("name") == "entry_links"]
>
> Another “find child tag by name”?  Please extract.
>

Extracted to find_by_attribute.

> +    def _check_entries(self, resource_type, entries):
> +        """Ensure that C{entries} are valid for a C{resource_type} collection.
> +
> +        @param resource_type: The resource type of the collection the entries
> +            are in.
> +        @param entries: A list of dicts representing objects in the
> +            collection.
> +        """
> +        name, child_resource_type = self._get_child_resource_type(resource_type)
> +        for entry in entries:
> +            self._check_resource_type(child_resource_type, entry)
> +        return name, child_resource_type
>
> Where did that return come from?  The docstring doesn't mention it and the method name doesn't suggest that there is one.

I've updated the docstring but not the method name. I don't think it's
worth chasing coherency of get vs check at this point.

> +class FakeRoot(FakeResource):
> +
> +    def __init__(self, application):
> +        """Create a L{FakeResource} for the service root of C{application}.
> +
> +        @param application: A C{wadllib.application.Application} instance.
> +        """
> +        resource_type = application.get_resource_type(
> +            application.markup_url + "#service-root")
> +        super(FakeRoot, self).__init__(application, resource_type)
>
> Here the class has no docstring but the __init__ has one containing the stuff that FakeResource had in the class docstring.  Was one of them a mistake?

One of them was written by me, the other by Jamu well over a year ago.
As mentioned, I've corrected the latter.

> === added file 'src/launchpadlib/testing/resources.py'
>
> +# Copyright 2008 Canonical Ltd.
>
> Why 2008?
>

That's how I found it. It's rude to change copyright years unless you
know what you're doing. I've appended 2011 to this.

>
> That's all I have time for tonight, I'm afraid.  I'll have to look into the tests tomorrow, or if you're in a rush, someone else may be able to do it.  They look strikingly different: short, punchy, well-explained.  Looking forward to them!
>

Well, thanks. I'm in a rush, because I'm trying to keep
work-in-progress down to a minimum. I'll try to find someone else.

jml