Code review comment for lp:~benji/lazr.restful/tweak-etag

Revision history for this message
Leonard Richardson (leonardr) wrote :

I have some minor comments, but overall this is a good branch.

In addition to the 'roman' package, you've got a normalizer that makes Unicode strings look the same as regular strings. I don't think that's the best way to solve your problem, but you should talk it over with Gary.

make_entry_etag_cores no longer cares whether a field "might change over time", so you should remove that bit from the docstring.

Similarly, you don't need "modifiable" in test_writable_fields and the similar tests. And you don't need test_modifiable_fields at all. It would be nice to test fields with this property, but you can't test them in a unit test of make_entry_etag_cores, which no longer looks at the 'modifiable' attribute.

Do you want to add a test to test_cores_change_with_value that shows a case where the write core changes and the read core stays the same?

« Back to merge proposal