Code review comment for lp:~thumper/launchpad/recipe-new-ppa

Hi Tim,

I'm happy with the code in this branch, with a few nitpicks. You need to
get a UI review too (and I'm not a UI reviewer). I've requested a UI review
for the MP.

82 + return render(index=term.value,
83 + text=label,
84 + value=value,
85 + name=widget.name,
86 + cssClass='')

This should be formatted as:

    return render(
        index=term.value, text=label, value=value, name=widget.name,
        cssClass='')

167 +USE_ARCHIVE_VOCABULARY = SimpleVocabulary((
168 + SimpleTerm(EXISTING_PPA, EXISTING_PPA,
169 + _("Use an existing PPA")),
170 + SimpleTerm(CREATE_NEW, CREATE_NEW,
171 + _("Create a new PPA for this recipe")),
172 + ))

This should be formatted:

USE_ARCHIVE_VOCABULARY = SimpleVocabulary((
    SimpleTerm(EXISTING_PPA, EXISTING_PPA, _("Use an existing PPA")),
    SimpleTerm(
        CREATE_NEW, CREATE_NEW, _("Create a new PPA for this recipe")),
    ))

183 + ppa_name = TextLine(
184 + title=_("New PPA name"), required=False,
185 + constraint=name_validator,
186 + description=_("A new PPA with this name will be created for "
187 + "the owner of the recipe ."))

The arguments here should be indented by one level from the declaration,
not two.

A couple of JS points that I'm not too worried about (but decided to
point out all the same):

444 + function set_enabled(field_id, is_enabled) {
445 + var field = Y.DOM.byId(field_id);
446 + field.disabled = !is_enabled;
447 + if (is_enabled && set_field_focus) field.focus();
448 + };

You don't need a semicolon here.

450 + module.onclick_use_ppa = function(e) {
451 + var value = getRadioSelectedValue('input[name=field.use_ppa]');
452 + if (value == 'create-new') {
453 + set_enabled(PPA_NAME_ID, true);
454 + set_enabled(PPA_SELECTOR_ID, false);
455 + }
456 + else {
457 + set_enabled(PPA_NAME_ID, false);
458 + set_enabled(PPA_SELECTOR_ID, true);
459 + }
460 + }

But technically you *do* need one here, since it's part of an
assignment. Ain't JS fun?