Code review comment for lp:~leonardr/lazr.restful/561521

Hi Leonard,

This branch looks good. I just have one comment below.

merge-conditional

-Edwin

>=== modified file 'src/lazr/restful/_resource.py'
>--- src/lazr/restful/_resource.py 2010-04-13 15:35:24 +0000
>+++ src/lazr/restful/_resource.py 2010-04-14 17:30:37 +0000
>@@ -1988,3 +1988,36 @@
> """The URL to the description of the object's full representation."""
> return "%s#%s-full" % (
> self._service_root_url(), self.singular_type)
>+
>+
>+def get_entry_fields_in_write_order(entry):
>+ """Return the entry's fields in the order they should be written to.
>+
>+ The ordering is intended to 1) be deterministic for a given schema
>+ and 2) minimize the chance of conflicts. Fields that are just
>+ fields come before fields (believed to be) controlled by
>+ mutators. Within each group, fields are returned in the order they
>+ appear in the schema.
>+
>+ :param entry: An object that provides IEntry.
>+ :return: A list of 2-tuples (field name, field object)
>+ """
>+ non_mutator_fields = []
>+ mutator_fields = []
>+ field_implementations = entry.__class__.__dict__
>+ for name, field in getFieldsInOrder(entry.schema):
>+ if name.startswith('_'):
>+ # This field is not part of the web service interface.
>+ continue
>+
>+ add_to = non_mutator_fields
>+ # If this field is secretly a subclass of lazr.delegates
>+ # Passthrough (but not a direct instance of Passthrough), put
>+ # it at the end -- it's probably controlled by a mutator.
>+ implementation = field_implementations[name]
>+ if (issubclass(implementation.__class__, Passthrough)
>+ and not implementation.__class__ is Passthrough):
>+ add_to = mutator_fields
>+ add_to.append((name, field))
>+ return non_mutator_fields + mutator_fields

Setting the add_to variable makes it a little harder to follow the
logic. I would prefer getting rid of it and using an if/else with
mutator_fields.append() and non_mutator_fields.append() instead.