Code review comment for lp:~rvb/launchpad/init-multiple-parents-packagesetpicker

Hi Jeroen,

Sorry about the fact that this code is not really readable. I've tried to improve it quite a bit.

> What is distroseries exactly, and more to the point, what is
> distroseries.value? It would be helpful to have some documentation of the
> parameters. Is distroseries.value a distroseries id?

Added documentation.

> The new code looks like it may be slower. Could that be an issue?

I don't think so, it'll just iterate over the packagesets of a distroseries.

> As far as I can tell, that's a CSS3 selector for "value ends with a dash
> followed by distroseries_id." Is it safe for us to use CSS3 selectors?
> AFAICT CSS2 does not have $=.
>
> The new code looks for:
>
> 'option[value$="' + packageset.get("id") + '"]'
>
> That means "value ends with the packageset id." I may be misunderstanding it,
> but it looks wrong to me in two ways:
>
> 1. So what does that value end in — distroseries id or packageset id!?
>
> 2. What about spurious substring matches, e.g. won't this also remove "321"
> when you really want to remove only "21"?

Ah! Thanks for spotting this! I've changed it to:

{{{
this.fieldNode.one(
    'option[value="' + packageset.get("id") + '"]').remove();
}}}

Just so you know, before this change, the packageset was identified by packagesetname-distro_id. Hence left-over/buggy the substring matches ... I guess staring at javascript too much made me dizzy.

> It would be helpful to say here what structure _packagesets will have, e.g.
> that it maps distroseries ids to arrays of packageset ids. We minify our JS,
> so comments won't slow the code down but they can save oodles of time in
> maintenance.

Good point, I've added a small explanation for this._packagesets.

> At 40 lines this test is far too long for me to see what it does without any
> comments or helpful identifiers. Is there anything you can do to express what
> this test tests?

I've refactored the test to make it more clear ... I hope. But I'm afraid js is kinda verbose :(.

> The substr('-5') is highly opaque. If there is no clearer way to write this
> (I can't guess because I have no indication what it's for) then can you
> isolate it in a function with a helpful name, or comment it?
> [...]
> Except that "name" may not actually be the distroseries name, because it's
> just the last 5 characters… in which case, why not use the full "ubuntu/hoary"
> and "ubuntu/breezy" here and drop the confusing "substr('-5')" trick?

Again, very good point, I refactored the code.

> Are the ids 4 and 5 here in any way related to the ids 4 and 5 you use above?
> If you're looking to check for possible confusion between the two, then that's
> a good thing to put in a separate test. Don't lob multiple aspects into a
> single test because they can grow, grow, grow without bound.

I've changed the ids to make it more clean. It was not my intention to test for that (and I don't think it's worth adding a test for this).

> I guess these are packageset ids. But the only thing that really tells me
> that is the "6"! It's hard to come up with anything that would make this
> clearer. Maybe you could wrap the widget.get("choices") in a function
> get_packageset_choices(widget)?

I've kept it this way (because it's the way it's tested everywhere in the code) but added a comment.

R.