launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

156	+ def get_template(name, from_template, default=False):
157	+ filenames = get_preseed_filenames(node, name, release, default)
158	+ filepath, content = get_preseed_template(filenames)
159	+ if filepath is None:
160	+ raise TemplateNotFoundError(name)
161	+ return PreseedTemplate(
162	+ content, name=filepath, get_template=get_template)
163	+
164	+ return get_template(prefix, None, default=True)

Can you please add comments explaining that get_template is a Tempita hook and that you need to preserve the context for it via the closure since the hook is called out of scope.

227	+ list(get_preseed_filenames(node, type, release, True)))

When passing boolean arguments, it's much more useful to the reader to use the keyword argument here so something like:
list(get_preseed_filenames(node, type, release, default=True)))
is far more readable, especially if more bool params are added later.

236	+ list(get_preseed_filenames(node, type, release, True)))

Same here and in the new tests.

275	+ def test_preseed_template_b64decode(self):
281	+ def test_preseed_template_b64encode(self):

Some comments on these, and on the 
130	+class PreseedTemplate(tempita.Template):
131	+ """A Tempita template specialised for preseed rendering."""

to explain why it's specialised would be very useful.  I can see it's something to do with b64 stuff but it's hard to tell exactly what at a glance.


-- 
https://code.launchpad.net/~rvb/maas/preseed-customisation/+merge/110861
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/preseed-customisation into lp:maas.