Code review comment for lp://qastaging/~thisfred/desktopcouch/list-to-mergeable-list-mapping

Revision history for this message
Nicola Larosa (teknico) wrote :

Code is good, tests pass.

We run the code through the debugger to understand the set_value method. At first we thought it was broken, but it is not, so good job. :-)

However, the docstring could stand some more verbosity. :-P

Here's how it could look:

"""
Set the value for the field, preserving the UUIDs as much as possible.

The value needs to be a sequence of elements.
If the field is not already present, it will be added and its value will be set
to a MergeableList containing the new elements.

If the field is already present, the current elements will be overwritten by
the new ones.

If the new elements are fewer than the current ones, the non-overwritten
current elements (the tail) will be dropped.

If the new elements are more than the current ones, the new ones in excess will
be appended to the list, and will get new UUIDs.
"""

Also, the APP_RECORD2 constant is not used (u1lint did not notice, weird). Its value should also be ['foo', 'bar'], to be in sync with TEST_RECORD2.

review: Approve

« Back to merge proposal