Code review comment for lp:~leonardr/lazr.restful/version-specific-request-interface

Hi Leonard,

Nice to see this coming along.

This is good to go once you fix the conflit marker and handle my few other
comments.

Cheers

> === modified file 'src/lazr/restful/docs/multiversion.txt'
> --- src/lazr/restful/docs/multiversion.txt 2009-11-19 16:43:08 +0000
> +++ src/lazr/restful/docs/multiversion.txt 2010-01-06 17:40:25 +0000
> @@ -5,10 +5,99 @@
> services. Typically these different services represent successive
> versions of a single web service, improved over time.
>
> +Setup
> +=====
> +
> +First, let's set up the web service infrastructure. Doing this first
> +will let us create HTTP requests for different versions of the web
> +service. The first step is to make all component lookups use the
> +global site manager.
> +
> + >>> from zope.component import getSiteManager
> + >>> sm = getSiteManager()
> +
> + >>> from zope.component import adapter
> + >>> from zope.component.interfaces import IComponentLookup
> + >>> from zope.interface import implementer, Interface
> + >>> @implementer(IComponentLookup)
> + ... @adapter(Interface)
> + ... def everything_uses_the_global_site_manager(context):
> + ... return sm
> + >>> sm.registerAdapter(everything_uses_the_global_site_manager)

Like discussed on IRC, the waving of this dead chicken isn't needed :-)

> +
> +Defining the request interfaces
> +-------------------------------
> +
> +Every version must have a corresponding subclass of
> +IWebServiceClientRequest. These marker interfaces let lazr.restful
> +keep track of which version of the web service a particular client
> +wants to use. In a real application, these interfaces will be
> +generated and registered automatically.
> +
> + >>> from lazr.restful.interfaces import (
> + ... IVersionedClientRequestImplementation, IWebServiceClientRequest)
> + >>> class IWebServiceRequestBeta(IWebServiceClientRequest):
> + ... pass
> +
> + >>> class IWebServiceRequest10(IWebServiceClientRequest):
> + ... pass
> +
> + >>> class IWebServiceRequestDev(IWebServiceClientRequest):
> + ... pass
> +
> + >>> request_classes = [IWebServiceRequestBeta,
> + ... IWebServiceRequest10, IWebServiceRequestDev]
> +
> + >>> from zope.interface import alsoProvides
> + >>> for cls, version in ((IWebServiceRequestBeta, 'beta'),
> + ... (IWebServiceRequest10, '1.0'),
> + ... (IWebServiceRequestDev, 'dev')):
> + ... alsoProvides(cls, IVersionedClientRequestImplementation)
> + ... sm.registerUtility(cls, IVersionedClientRequestImplementation,
> + ... name=version)
> +

Can you explain the use of IVersionedClientRequestImplementation? You
explained on IRC that it to make it easy to retrieve the interface related to
a version string.

Would IVersionedWebClientRequestFactory be a better name?

> +<<<<<<< TREE
> >>> for version in ['beta', 'dev', '1.0']:
> ... sm.registerAdapter(
> ... ContactCollection, provided=ICollection, name=version)
> @@ -397,3 +495,105 @@
> >>> print absoluteURL(dev_app, dev_request)
> http://api.multiversion.dev/dev/
>
> +=======

You have a conflict marker here.

> +Collections
> +===========
> +
> + >>> request = create_web_service_request("/beta/contact_list")
> + >>> request.traverse(None)
> + <...CollectionResource object at...>
> +>>>>>>> MERGE-SOURCE

Which ends here.

> === modified file 'src/lazr/restful/interfaces/_rest.py'

> class IWebServiceClientRequest(IBrowserRequest):
> - """Marker interface requests to the web service."""
> + """Interface for requests to the web service."""
> + version = Attribute("The version of the web service that the client "
> + "requested.")

Does this branch introduce that attribute? If it does, where is the doctest
example showing it's use?

> === modified file 'src/lazr/restful/publisher.py'
> --- src/lazr/restful/publisher.py 2009-11-19 15:53:26 +0000
> +++ src/lazr/restful/publisher.py 2010-01-06 17:40:25 +0000
> @@ -34,7 +34,12 @@
> EntryResource, ScopedCollection, ServiceRootResource)
> from lazr.restful.interfaces import (
> IByteStorage, ICollection, ICollectionField, IEntry, IEntryField,
> +<<<<<<< TREE
> IHTTPResource, IServiceRootResource, IWebBrowserInitiatedRequest,
> +=======
> + IHTTPResource, IServiceRootResource,
> + IVersionedClientRequestImplementation, IWebBrowserInitiatedRequest,
> +>>>>>>> MERGE-SOURCE
> IWebServiceClientRequest, IWebServiceConfiguration)

Another conflict marker.

> # This object should not be published on the web service.
> + import pdb; pdb.set_trace()
> raise NotFound(ob, '')

Stray debugging.

> @@ -255,6 +261,7 @@
> raise NotFound(self, '', self)
> self.annotations[self.VERSION_ANNOTATION] = version
>
> +<<<<<<< TREE
> # Find the appropriate service root for this version and set
> # the publication's application appropriately.
> try:
> @@ -265,6 +272,32 @@
> service_root = getUtility(IServiceRootResource)
> self.publication.application = service_root
>
> +=======

More conflict markers.