Code review comment for lp:~mkanat/loggerhead/view-default

Revision history for this message
Max Kanat-Alexander (mkanat) wrote :

> I think it's mostly fine. The template is ugly though. Why do you need to
> put a None at the end of the annotation iterator? It would be better to pad
> out the results until you've reached the length of the text lines, or
> something.

  That's true, I could do that, but that would also then involve code for keeping track of when there were extra lines in 'contents' that were beyond how far along we were with annotating. Would simplify the template, though, which I think is a valid goal. I think the problem is that 'contents' contains an extra final line when there's a "\n" at the end of the file, and annotate_iter doesn't consider that to be a line.

> If you're going to rename the annotation css file, you should probably rename
> the rules therein too.

  Yeah, I suppose you're right. I was trying to keep the changes at least *somewhat* focused though, so that they could be reviewed sensibly. But now that it's passed review I'll just do that change.

> Everything else looks ok. Loading the annotation information via ajax would
> be nice, but can wait :-)

  Yeah, I had the same thought.

« Back to merge proposal