Code review comment for lp:~leonardr/lazr.restful/optional-compression

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi Leonard,

This branch looks good. I just have two comments below.

merge-approved

-Edwin

>=== modified file 'src/lazr/restful/interfaces/_rest.py'
>--- src/lazr/restful/interfaces/_rest.py 2009-09-08 12:40:55 +0000
>+++ src/lazr/restful/interfaces/_rest.py 2009-10-14 18:14:57 +0000
>@@ -454,6 +454,20 @@
> "a collection, they will not be allowed to request more entries "
> "in the batch than this.")
>
>+ set_hop_to_hop_headers = Bool(
>+ title=u"Whether your application server will allow lazr.restful "
>+ "to set hop-top-hop headers.",

s/-top-/-to-/
I had never heard of hop-to-hop before, so I looked it up. It
seems that it is normally referred to as hop-by-hop. It is reasonably
clear as-is, so this is not a requirement, just a suggestion.

>+ default=True,
>+ description=u"Ordinarily, lazr.restful automatically compresses "
>+ "outgoing representations and sets the Transfer-Encoding "
>+ "header accordingly. (Apache's mod_compress does something "
>+ "similar, but it sets the Content-Encoding header, which means "
>+ "it has to munge the ETag header, which is no good for our purposes.) "
>+ "But WSGI application servers prohibit intermediaries from "
>+ "setting hop-to-hop headers like Transfer-Encoding. The best you can "
>+ "do in that situation is avoid setting those headers and use a "
>+ "web server that does compression using Transfer-Encoding.")
>+
> def createRequest(body_instream, environ):
> """A factory method that creates a request for the web service.
>
>
>=== modified file 'src/lazr/restful/wsgi.py'
>--- src/lazr/restful/wsgi.py 2009-10-07 12:53:04 +0000
>+++ src/lazr/restful/wsgi.py 2009-10-14 18:14:57 +0000
>@@ -2,6 +2,7 @@
>
> __metaclass__ = type
> __all__ = [
>+ 'BaseWSGIWebServiceConfiguration',
> 'WSGIApplication',
> ]
>
>@@ -15,7 +16,8 @@

pyflakes says this file has an unused import.
    from zope.interface import implements

>
> from lazr.restful.interfaces import (
> IWebServiceConfiguration, IServiceRootResource)
>-from lazr.restful.simple import Publication, Request
>+from lazr.restful.simple import (
>+ BaseWebServiceConfiguration, Publication, Request)
>
>
> class WSGIApplication:
>@@ -60,3 +62,12 @@
> """Create a WSGI server object for a particular web service."""
> cls.configure_server(host, port, config_package, config_file)
> return wsgi_make_server(host, int(port), cls)
>+
>+
>+class BaseWSGIWebServiceConfiguration(BaseWebServiceConfiguration):
>+ """A basic web service configuration optimized for WSGI apps.
>+
>+ The only difference between this and the default
>+ WebServiceConfiguration is that set_hop_to_hop_headers is False.
>+ """
>+ set_hop_to_hop_headers = False

review: Approve (code)

« Back to merge proposal