Merge lp:~leonardr/lazr.restful/its-really-read-only into lp:lazr.restful

[Leonard Richardson]

This simple branch makes it possible to take a read-write field and publish it as read-only in the web service. I tested this with a simple doctest, and by changing the integration test example web service so that ICookbook.copyright_date was a read-write field published as read-only, rather than a simple read-only field.

The opposite of this branch is taking a read-only field and publishing it as read-write. This was already supported by the use of mutators. That's a much more complicated situation and there's no need to do anything nearly as complicated here.

[Guilherme Salgado (salgado)]

Hi Leonard,

This branch looks fine; I have just one minor suggestion.

 review approve
 status approved

On Thu, 2010-08-26 at 18:48 +0000, Leonard Richardson wrote:
[...]
> === modified file 'src/lazr/restful/declarations.py'
> --- src/lazr/restful/declarations.py 2010-08-18 18:02:44 +0000
> +++ src/lazr/restful/declarations.py 2010-08-26 18:48:44 +0000
> @@ -213,14 +213,13 @@
> annotation_stack['type'] = FIELD_TYPE
>
> annotation_key_for_argument_key = {'exported_as' : 'as',
> - 'exported' : 'exported'}
> + 'exported' : 'exported',
> + 'readonly' : 'readonly'}
>
> - # The only valid keyword parameters are 'exported' and
> - # 'exported_as', which manage the behavior in the first
> - # version. If these keywords are present, incorporate them into
> - # the VersionedDict.
> - for key in ('exported_as', 'exported'):
> - annotation_key = annotation_key_for_argument_key[key]
> + # If keyword parameters are present, they define the field's
> + # behavior for the first version. Incorporate them into the
> + # VersionedDict.
> + for (key, annotation_key) in annotation_key_for_argument_key.items():
> if key in kwparams:
> annotation_stack[annotation_key] = kwparams.pop(key)
> # If any keywords are left over, raise an exception.
> @@ -918,6 +917,19 @@
> mutated_by, mutated_by_annotations = tags.get(
> 'mutator_annotations', (None, {}))
> readonly = (field.readonly and mutated_by is None)
> + # Clear up some possible confusion.
> + if (readonly and tags.get('readonly', None) == False):
> + raise TypeError(
> + ("%s.%s is defined as a read-only field and you "
> + "haven't defined a mutator for it. You can't just "
> + "declare it to be read-write in the web service: "
> + "you need to define a mutator.") % (
> + interface.__name__, field.__name__))
> + if not readonly and tags.get('readonly', False):
> + # The field is read-write internally, but the
> + # developer wants it to be read-only through the web
> + # service.
> + readonly = True

There's something about using tags.get() twice with different arguments
that makes this code (IMHO) not as easy to follow as it should be. Maybe
it's just me, but I thought of the following, which certainly makes it
look clearer:

        if 'readonly' in tags:
            publish_as_readonly = tags.get('readonly')
            if readonly and not publish_as_readonly:
                raise TypeError(...)
            if not readonly and publish_as_readonly:
                readonly = True

What do you think?

> attrs[tags['as']] = copy_field(
> field, __name__=tags['as'], readonly=readonly)
> class_name = _versioned_class_name(

review: Approve

[lp:~leonardr/lazr.restful/its-really-read-only]

143. By Leonard Richardson on 2010-08-26

Clarified the code around declaring a read-write field read-only through the web service.