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

Revision history for this message
Ian Booth (wallyworld) wrote :

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

Done

> 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']
>
<snip>

I may be wrong - RFC2616 says
"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma"

Since the notification text values may be escaped html and totally free form text, it's not possible I think to do what is suggested above. The only way I can see at this point is to encode the notification text values as a json string and send as one header value.

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

Removed from examples etc and just retained the unit test.

« Back to merge proposal