Code review comment for lp:~wallyworld/lazr.restful/propogate-notifications

Revision history for this message
Benji York (benji) wrote :

There are a couple of things that are still needed on this branch.

First, I can't get the tests to pass. First there was an undeclared
dependency on testtool, so I removed references to it but then I got
many, many other failures. If you're not seeing these, then I suspect
you're running the tests with a dirty Python (e.g., a system Python).
Running with a clean Python (or virtualenv) should result in the same
failures.

The INotificationsProvider interface is just a marker (i.e., it has not
attributes or methods). It looks like it should have a "notifications"
attribute. Something like this should do:

=== modified file 'src/lazr/restful/interfaces/_rest.py'
--- src/lazr/restful/interfaces/_rest.py 2011-03-25 08:10:16 +0000
+++ src/lazr/restful/interfaces/_rest.py 2011-03-29 20:26:35 +0000
@@ -56,6 +56,8 @@
     'IWebServiceVersion',
     ]

+from textwrap import dedent
+
 from zope.schema import Bool, Dict, Int, List, Text, TextLine
 from zope.interface import Attribute, Interface
 # These two should really be imported from zope.interface, but
@@ -281,16 +283,20 @@
     """A response object which contains notifications.

     A web framework may define an adapter for this interface, allowing
- the web service to provide an implementation having a 'notifications'
- property, providing a list of namedtuples (level, message) which can be
- used by the caller to display extra information about the completed
- request. 'level' matches the standard logging levels:
- DEBUG = logging.DEBUG # A debugging message
- INFO = logging.INFO # simple confirmation of a change
- WARNING = logging.WARNING # action will not be successful unless you ...
- ERROR = logging.ERROR # the previous action did not succeed, and why
+ the web service to provide notifications to be sent to the client.
     """

+ notifications = Attribute(dedent("""\
+ A list of namedtuples (level, message) which can be used by the
+ caller to display extra information about the completed request.
+ 'level' matches the standard logging levels:
+
+ DEBUG = logging.DEBUG # a debugging message
+ INFO = logging.INFO # simple confirmation of a change
+ WARNING = logging.WARNING # action will not be successful unless...
+ ERROR = logging.ERROR # the previous action did not succeed
+ """))
+

Now for lesser things:

A couple of the multi-line imports in src/lazr/restful/publisher.py need
trailing commas.

Here's a small whitespace fix:

=== modified file 'src/lazr/restful/publisher.py'
--- src/lazr/restful/publisher.py 2011-03-28 00:09:43 +0000
+++ src/lazr/restful/publisher.py 2011-03-29 20:31:53 +0000
@@ -201,7 +201,7 @@
             and notifications_provider.notifications):
             notifications = ([(notification.level, notification.message)
                  for notification in notifications_provider.notifications])
- json_notifications =simplejson.dumps(notifications)
+ json_notifications = simplejson.dumps(notifications)
         request.response.setHeader(
             'X-Lazr-Notifications', json_notifications)

review: Needs Fixing (code)

« Back to merge proposal