Code review comment for lp:~wallyworld/launchpad/inline-recipe-distro-series-edit

Revision history for this message
Ian Booth (wallyworld) wrote :

> This looks pretty good.
>
> I wonder whether label_tag="dt" and items_tag="dd" could be defaults.
>

Yeah, thought about that but with a sample size of 1 - this is the only place it's used so far - wasn't sure about whether to encode the assumption the widget would be rendered in a definition list or not. In the end, I thought it safer to go with span. We can always revisit when we get more places using the widget.

> Sorry for using distros rather than distroseries. I was new to the problem
> domain.
>

No probs.

> I don't think you should have a setUp in RecipeEdit until have more than one
> test, because until you have more tests, you won't know what needs to vary.
>

Ok. Will fix.

> I also see you setting up far more constants than you actually use, e.g.
> recipe name. Why not just use makeSourcePackageRecipe? (you could use "with
> person_logged_in(recipe.owner)".
>

Will take alook.

> The implementation of updateSeries is a bit sad. Is there no straightforward
> way to update that?

Ah, the way that is there is absolutely required if the distroseries field is exported as a CollectionField, which it was initially. However, now that the distroseries field is a List, that approach may not be required anymore. I'll have a look.

« Back to merge proposal