Code review comment for lp:~barry/lazr.delegates/lp1096513

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good. Some minor points. I did a fairly quick once-over
sanity-check rather than an in-depth review, so you might want to ask
someone else to take a look if you think it warrants it.

[1]

+def delegate_to(*interfaces, **kws):
+    context = kws.pop('context', 'context')
+    if len(kws) > 0:
+        raise TypeError('Too many arguments')
+    def _decorator(cls):
+        if len(interfaces) == 0:
+            raise TypeError('At least one interface is required')

I think it would make more sense to check interfaces at the same time
as kws, i.e. in delegate_to, rather than _decorator.

In both _python2.py and python3.py.

[2]

--- lazr/delegates/docs/fixture.py 1970-01-01 00:00:00 +0000
+++ lazr/delegates/docs/fixture.py 2013-01-07 10:44:28 +0000
@@ -0,0 +1,34 @@
+# Copyright 2009-2013 Canonical Ltd.  All rights reserved.
+#
+# This file is part of lazr.smtptest

s/smtptest/delegates/

In lazr/delegates/docs/usage_fixture.py too.

[3]

+The ``@delegation`` decorator makes a class implement zero or more interfaces

s/delegation/delegate_to/

[4]

=== added file 'lazr/delegates/docs/usage_fixture.py'
...
+__all__ = [
+    'globs',
+    ]
+
+
+from lazr.delegates.docs.fixture import globs

What does this module do?

[5]

+class TestAPI(unittest.TestCase):
+    """Test various corner cases in the API."""
+
+    def test_no_interfaces(self):
+        try:
+            @delegate_to()
+            class SomeClass(object):
+                pass
+        except TypeError:
+            pass
+        else:
+            raise AssertionError('TypeError expected')

assertRaises would be more concise, but I can see that there's some
clarity here from using decorator syntax. Use self.fail() instead of
`raise AssertionError`?

review: Approve

« Back to merge proposal