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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
