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

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

> OK, I'm convinced that there's no simpler solution right now than JSON-
> encoding the notifications and putting them in a single header. The
> implementation looks good as well.
>
> The only suggestion I can make is that you should be able to put a single
> _processNotifications() call in WebServicePublicationMixin.callObject(),
> instead of calling it from within every implementation of __call__. This would
> centralize the notification code so it's only called once, and it would also
> put it 'outside' of the resource processing, in the request processing. This
> fits nicely with the way we moved the notifications themselves 'outside' of
> the resource representations and into the HTTP response.

I've moved the notifications processing code as suggested to WebServicePublicationMixin and tweaked the unit test accordingly. As discussed via mumble, I'm keeping the version as 0.18.1 and once approved, the new package will be released as such under this version number. I've changed the status to Needs Review.

« Back to merge proposal