Code review comment for lp:~leonardr/lazr.restful/forbid-reference-to-entry-not-published-in-this-version

Revision history for this message
Tim Penhey (thumper) wrote :

A simple docstring for webservice_sanity_checks would be nice.

Perhaps a dict string mapping would be better here:
    raise ValueError(
        "In version %s, %s.%s is a Reference to %s, "
        "but version %s of the web service does not publish "
        "%s as an entry. (It may not be published "
        "at all.)" % (
            version, interface.__name__, field.__name__,
            referenced_interface.__name__, version,
            referenced_interface.__name__))
becomes
    raise ValueError(
        "In version %(version)s, %(interface)s.%(field)s is a Reference "
        "to %(reference)s, but version %(version)s of the web service "
        "does not publish %(reference)s as an entry. "
        "(It may not be published at all.)" % {
            'version': version,
            'field': field.__name__,
            'interface': interface.__name__,
            'reference': referenced_interface.__name__}

For your assert raises test, why not grab the copy from testtools?

    def assertRaises(self, excClass, callableObj, *args, **kwargs):
        """Fail unless an exception of class excClass is thrown
           by callableObj when invoked with arguments args and keyword
           arguments kwargs. If a different type of exception is
           thrown, it will not be caught, and the test case will be
           deemed to have suffered an error, exactly as for an
           unexpected exception.
        """
        try:
            ret = callableObj(*args, **kwargs)
        except excClass:
            return sys.exc_info()[1]
        else:
            excName = self._formatTypes(excClass)
            self.fail("%s not raised, %r returned instead." % (excName, ret))

You'd need this too:

    def _formatTypes(self, classOrIterable):
        """Format a class or a bunch of classes for display in an error."""
        className = getattr(classOrIterable, '__name__', None)
        if className is None:
            className = ', '.join(klass.__name__ for klass in classOrIterable)
        return className

test_reference_to_object_published_later_fails exception string test
will always pass as you are only checking that the string isn't empty.

On the whole I think the implementation is fine.

review: Needs Fixing

« Back to merge proposal