Code review comment for lp:~lucio.torre/graphite/add-events

Revision history for this message
Sidnei da Silva (sidnei) wrote :

Looks pretty ok to me. A few comments, but none of them blockers:

[1] arrays_equal has a few issues the way it's implemented, but should work given how it's used. There's a really nice array comparison implementation here: http://www.breakingpar.com/bkp/home.nsf/0/87256B280015193F87256BFB0077DFFD

[2] I'm worried about the use of 'autoescape off' here, since basically you could inject some javascript through post_event handler and have it rendered unsanitized into the template, but Lucio pointed out there's more templates already using 'autoescape off' in the tree. Probably deserves some checking all around. It doesn't seem like removing it from those newly introduced templates would break them though.

review: Approve

« Back to merge proposal