Code review comment for lp:~michael.nelson/launchpad/distro-series-difference-browser2

Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Tue, Sep 7, 2010 at 9:42 PM, Leonard Richardson
<email address hidden> wrote:
> Review: Approve
> It's been a while since I did anything with TALES, but I think I was able to follow this. I approve with only minor comments:

Thanks Leonard,

>
> line 48 "betteen"

Done

> Can 58 and 59 fit on one line?

Indeed.

> Why are lines 134 and 135 commented out? It seems like an important part of the test.

Indeed - it was a mistake. I've uncommented and reworked that line to
check that the flag is not set.

> Is it really OK to have the python: call on line 328? I remember that being discouraged.

It's OK if it's necessary - I couldn't find a way in TALES to index an
iterable (nor think of a better way to provide this via the view - ie.
construct a dict of first comments for all differences in the batch?
erg)... if you know one, please let me know.

Here's the incremental diff with the changes you recommended:

http://pastebin.ubuntu.com/490231/

Now onto the UI tweaks... thanks Leonard!

« Back to merge proposal