Merge lp://staging/~barry/lazr.delegates/lp1096513 into lp://staging/lazr.delegates

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

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

On Jan 08, 2013, at 12:28 PM, Gavin Panella wrote:

>> BTW, what did you think of @delegate_to as the choice of name?
>
>It's good, and I can't think of anything better.
>
>...
>> Would you take a quick look at the lazr.smtptest mp?
>> https://code.launchpad.net/~barry/lazr.smtptest/lp1096475
>
>I was too late :)

Yeah, it's futile to try to beat Curtis. :)

Thanks!

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

It's good, and I can't think of anything better.

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

I was too late :)