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

On Jan 07, 2013, at 11:02 AM, Gavin Panella wrote:

>Review: Approve
>
>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.

Thanks for the review! You caught some great things.

I'll see if Gary or Curtis wants to take a look, otherwise I'll JFDI :).

BTW, what did you think of @delegate_to as the choice of name?

>+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.

Good idea, fixed.

>--- 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.

Fixed everywhere.

>+The ``@delegation`` decorator makes a class implement zero or more interfaces
>
>s/delegation/delegate_to/

Fixed.

>=== added file 'lazr/delegates/docs/usage_fixture.py'
>...
>+__all__ = [
>+    'globs',
>+    ]
>+
>+
>+from lazr.delegates.docs.fixture import globs
>
>What does this module do?

It's a nosetests artifact. AFAICT, under nose (which is generally pretty
awesome) the only way to add a fixture for a doctest is to provide a suffix
which is appended to the doctest base name. So, given "_fixture" as the
suffix, for usage.rst it will only search for usage_fixture.py.

In lazr.delegates case, that's not a big deal, but I copied this over from
lazr.smtptest (which I'm porting to Python 3 in a very similar way) and it has
more than one doctest. So the idea was to put the common fixtures in a shared
module, e.g. lazr.delegates.doc.fixture, and then import what's needed into
the nose-required module name.

BTW, I opened a bug on nose for making this nicer.

https://github.com/nose-devs/nose/issues/594

>+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`?

Yeah, I did it this way to use the syntax that you'd see more commonly in the
code. Good idea about self.fail() though - fixed!

Update pushed, and again, thanks!

Would you take a quick look at the lazr.smtptest mp?
https://code.launchpad.net/~barry/lazr.smtptest/lp1096475