Code review comment for lp:~leonardr/lazr.restful/multiversion-collection

On January 11, 2010, Leonard Richardson wrote:
> Thanks for your thorough review. I'm going through it slowly. I'll probably
> have many comments in reply.
>
> > > # We are given a model schema (IFoo). Look up the
> > > # corresponding entry schema (IFooEntry).
> > > model_schema = self.relationship.value_type.schema
> > > - return getGlobalSiteManager().adapters.lookup1(
> > > - model_schema, IEntry).schema
> > > + request_interface = getUtility(
> > > + IVersionedClientRequestImplementation,
> > > + name=self.request.version)
> > > + return getGlobalSiteManager().adapters.lookup(
> > > + (model_schema, request_interface), IEntry).schema
> >
> > In my previous review, I suggested I...Factory instead of
> > IVersionedClientRequestImplementation, here I see that it's not a
> > factory. Sine you are using the name, why do you need a separate
> > interface. Why not simply ask for the IWebClientRequest with the specific
> > name? Since all versioned interface should extend that one, it should
> > work.
>
> I don't do that because request objects shouldn't be utilities. There's not
> one request object for every version, but one object for every request. I
> could create a dummy request object and register that as a utility, but I
> think that would be really confusing. What I have is also confusing, but
> it can be explained in an internally consistent way:
>
> 1. We need a way to look up a web service description given a version
> number. 2. These lookups don't operate by version number; they take a
> marker interface that's different for every version. 3. So we create a way
> to look up the marker interface given the version number. 4. We implement
> this by registering the marker interface objects as named utilities.
>
> Does this make sense?

Yes, this does make sense. And I do think you should be registering the marker
interface themselve. Remember that a class (or an interface) is an object in
itself. It's already a pattern in zope, the ZCA registers all interfaces as
utility already.

You are also right that what I suggest doesn't work, since the
IWebServiceRequest subclass don't provide IWebServiceRequest themselves. So
what you are doing is the proper way to do. I think I just don't like the name
(it's too long, and the utility you are retrieving isn't an implementation.)
How about IWebServiceVersion ?

>
> I believe in all these cases we have a specific request object whose
> .version we look up. If there was some way of extracting from the request
> object the marker interface that identified the version, we could write a
> property method WebServiceClientRequest.marker_interface. Is that what you
> meant? (I could write this property method now, with the utility lookup,
> and it would clean up the code a bit.)
>

--
Francis J. Lacoste
<email address hidden>