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

Revision history for this message
Leonard Richardson (leonardr) wrote :

I like how this is shaping up. The test in test_webservice.py is great. Here are my comments on the design:

1. Custom HTTP headers need to start with X-, and by convention our custom HTTP headers start with X-Lazr-. So XHR-Notifications should be X-Lazr-XHR-Notifications. But there's nothing XHR-specific about these notifications. That's how you're using them now, but other clients could use them for other purposes. So I recommend just X-Lazr-Notification.

2. The more complicated the value of the HTTP header, the less comfortable I feel. My ideal is for HTTP header values to be plain text. If client support allows it, I would prefer to see each notification sent in a separate copy of the header:

X-Lazr-Notification: ['DEBUG', 'Notification 1']
X-Lazr-Notification: ['WARN', 'Notification 2']

Or, even better:

X-Lazr-Notification: DEBUG: Notification 1
X-Lazr-Notification: WARN: Notification 2

Or:

X-Lazr-Notification-DEBUG: Notification 1
X-Lazr-Notification-WARN: Notification 2

It's up to you--just keep in mind that non-Ajax clients might want this information as well.

3. I don't think you need an INotificationsProvider implementation in simple.py. It's a really simple class and any real implementation needs a source for the notifications anyway. If you want to use it in the example/base doctests (see below), you could

4. You register a notification adapter in example/base, but you don't use it in any of the example/base doctests. There's no reason to put something in the example web service if you don't demonstrate the functionality. You can either take out the notification adapter (along with the simple.py implementation), or you can add a demonstration to the example/base doctests. For an optional feature like this, I think a unit test is sufficient.

« Back to merge proposal