launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Hi Māris,

Thanks for the review.


>  - on line 37, it might read easier as "for_distro = distroseries is not None
> or sourcepackagename is not None"

Started out with that; I found that combining "or" with the two negations made things harder, not easier.  Maybe they made me practice the De Morgan transformations too much in university.


>  - on line 39, the if statement would read better if you replaced the
> "potemplate is not None" test with a new "has_template" flag.

Done.


>  - Is there a more central place that we could put the escape_js_string()
> helper?  Perhaps in tales.py?

First I wanted to see how general this really was.  I suspect we won't often need to generate string constants in the browser code to go into JS through TAL.


>  - Why do you create an _escapeJSString() method instead of using
> escape_js_string()?

Whoops.  That's an older incarnation of escape_js_string that I wasn't actually using anymore.  Removed.


>  - in importqueueentry.js you can remove the @method JSDoc tags from the
> functions, because they are not methods :)  Check the JSDoc specs for a
> substitute.

Thanks for clearing that up.  I did look at the JSDoc specs but found nothing better, and with the emergent nature of the distinction between methods and functions in javascript, was a bit uncertain as to what was wanted.


>  - I think test_import_queue_entry_template_selection_escape() should be
> rewritten as a unit test against the template contents (or API).  Then the
> test would run much much much faster.

I saw this one as an acceptance test: I know the escaping works, but does it work as expected all along the line from python to TAL to XHTML to JavaScript to DOM?  It's easy to forget an escaping level or stack on one too many, and so I'm loath to give up the end-to-end certainty that this test gives me.


Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-600673/+merge/30679
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-600673 into lp:launchpad/devel.