Code review comment for lp://qastaging/~gmb/lp2kanban/dont-rely-on-external_system_url

Revision history for this message
Benji York (benji) wrote :

This branch is eminently landable as-is. I had a couple of
questions/thoughts while reading it.

I'm not quite clear on why the annotations aren't just a read/write
record or dictionary-like object. The "this protects devs from data
loss" bit confuses me.

I'm torn between the current implementation for storing the values and a
JSON-based format.

The current approach is nice to look at and edit. The benefit of JSON
would be that it is richer (arrays, objects, embedded newlines, etc.).

If we decided to go with JSON my first suggestion would be to divide the
description into "text before the first {", "text after the last }", and
"text between the first { and last } (including the braces themselves".
That approach would make it easy to preserve the text around the JSON
and using Python's "json" module would make the (un)serializing easy.

The tests look really good.

review: Approve (code)

« Back to merge proposal