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

Hi Leonard,

This is a nice refactoring and improvement. I only have minor comments below.

merge-conditional

-Edwin

>=== modified file 'src/lazr/restful/declarations.py'
>--- src/lazr/restful/declarations.py 2010-01-26 20:05:25 +0000
>+++ src/lazr/restful/declarations.py 2010-01-27 22:49:27 +0000
>@@ -202,7 +202,7 @@
> collection, or if used more than once in the same interface.
> """
>
>- def __init__(self, **params):
>+ def __init__(self, version=None, **params):
> """Create the decorator marking the default collection method.
>
> :param params: Optional parameter values to use when calling the

The docstring should explain how the version parameter should be used.

>@@ -891,20 +898,26 @@
> return method(**params)
>
>
>-def generate_collection_adapter(interface):
>+def generate_collection_adapter(interface, version=None):
> """Create a class adapting from interface to ICollection."""
> _check_tagged_interface(interface, 'collection')
>
> tag = interface.getTaggedValue(LAZR_WEBSERVICE_EXPORTED)
>- method_name = tag['collection_default_content']
>+ default_content_by_version = tag['collection_default_content']
>+ assert (version in default_content_by_version), (
>+ "'%s' isn't tagged for export to web service "
>+ "version '%s'." % (interface.__name__, version))
>+ method_name, params = default_content_by_version[version]
> entry_schema = tag['collection_entry_schema']
> class_dict = {
> 'entry_schema' : CollectionEntrySchema(entry_schema),
>- 'method_name': tag['collection_default_content'],
>- 'params': tag['collection_default_content_params'],
>+ 'method_name': method_name,
>+ 'params': params,
> '__doc__': interface.__doc__,
> }
>- classname = "%sCollectionAdapter" % interface.__name__[1:]
>+ classname =_versioned_class_name(

s/=_/= _/

>+ "%sCollectionAdapter" % interface.__name__[1:],
>+ version)
> factory = type(classname, (BaseCollectionAdapter,), class_dict)
>
> protect_schema(factory, ICollection)
>@@ -1016,8 +1029,9 @@
> if return_type is None:
> return_type = None
>
>- name = '%s_%s_%s_%s' % (prefix, method.interface.__name__, tag['as'],
>- version)
>+ name = _versioned_class_name(
>+ '%s_%s_%s' % (prefix, method.interface.__name__, tag['as']),
>+ version)
> class_dict = {'params' : tuple(tag['params'].values()),
> 'return_type' : return_type,
> '_export_info': tag,
>@@ -1026,8 +1040,20 @@
>
> if tag['type'] == 'write_operation':
> class_dict['send_modification_event'] = True
>- factory = type(make_identifier_safe(name), bases, class_dict)
>+ factory = type(name, bases, class_dict)
> classImplements(factory, provides)
> protect_schema(factory, provides)
>
> return factory
>+
>+
>+def _versioned_class_name(base_name, version):
>+ """Create a class name incorporating the given version string."""
>+ if version is None:
>+ # We need to incorporate the version into a Python class name,
>+ # but we won't find out the name of the earliest version until
>+ # runtime. Use a generic string that won't conflict with a
>+ # real version string.
>+ version = "__Earliest"
>+ name = "%s_%s" % (base_name, version)
>+ return make_identifier_safe(name)
>
>=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
>--- src/lazr/restful/docs/webservice-declarations.txt 2010-01-25 19:26:30 +0000
>+++ src/lazr/restful/docs/webservice-declarations.txt 2010-01-27 22:49:27 +0000
>@@ -170,15 +170,14 @@
> tagged value.
>
> >>> print_export_tag(IBookSet)
>- collection_default_content: 'getAllBooks'
>- collection_default_content_params: {}
>+ collection_default_content: {None: ('getAllBooks', {})}
> collection_entry_schema: <InterfaceClass __builtin__.IBook>
> type: 'collection'
>
> >>> print_export_tag(ICheckedOutBookSet)
>- collection_default_content: 'getByTitle'
>- collection_default_content_params: {'title': '',
>- 'user': <object...>}
>+ collection_default_content: {None: ('getByTitle',
>+ {'title': '',
>+ 'user': <object object ...>})}
> collection_entry_schema: <InterfaceClass __builtin__.IBook>
> type: 'collection'
>
>@@ -223,8 +222,8 @@
> ... """Another getAll()."""
> Traceback (most recent call last):
> ...
>- TypeError: only one method should be marked with
>- @collection_default_content.
>+ TypeError: Only one method can be marked with
>+ @collection_default_content for version (earliest).

I think this could actually confuse someone, since the parens don't
imply that it is actually the name of the version. It could also be
confusing if @collection_default_content(version="method") yielded the
error message ending with "for version method". I think double-quotes
around the version name would make it clear that the word doesn't have
another meaning in the sentence.

> export_as_webservice_collection() can only be used on Interface.
>
>@@ -1452,7 +1451,6 @@
> It is possible to cache a server response in the browser cache using
> the @cache_for decorator:
>
>- >>> from lazr.restful.testing.webservice import FakeRequest
> >>> from lazr.restful.declarations import cache_for
> >>>
> >>> class ICachedBookSet(IBookSet):