launchpad-reviewers team mailing list archive

[Māris Fogels <mfogels@xxxxxxxxx>]

Review: Approve python, js
Hi Jeroen,

That is quite the patch, and a good deal of work went into it.  It looks good!  I only have minor questions about the diff you pasted below:

 - on line 37, it might read easier as "for_distro = distroseries is not None or sourcepackagename is not None"
 - on line 39, the if statement would read better if you replaced the "potemplate is not None" test with a new "has_template" flag.
 - Is there a more central place that we could put the escape_js_string() helper?  Perhaps in tales.py?
 - Why do you create an _escapeJSString() method instead of using escape_js_string()?
 - 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.
 - 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.

That's all.  With those changes considered, r=mars.

Maris
-- 
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.