Code review comment for lp:~julian-edwards/maas/dup-boot-source-selections-part2-bug-1360280

Revision history for this message
Julian Edwards (julian-edwards) wrote :

On Wednesday 12 Nov 2014 07:37:38 you wrote:
> I can *hear* jtv, here and elsewhere, saying "What is this obsession with
> creating 3 _things_?"

Arf :)

> Also, I don't actually understand from the test why you're creating three
> mostly identical BootSourceCaches, here and elsewhere. What does it
> actually achieve? Wouldn't one do just as well?

You figured this out....

> > + boot_caches.append(factory.make_BootSourceCache(
> > + boot_source, arch=factory.make_name('arch'),
> > + os=os, release=release))
> > +
> > + params = {
> > + 'os': os,
> > + 'release': release,
> > + 'arches': [boot_caches[0].arch, boot_caches[2].arch],
>
> Ah, I get it now; you're combining the valid values for the field you're
> testing on from some but not all of the BootSourceCaches you've created.
>
> Consider moving the for loop into a helper. Also, add a comment (or a
> docstring on the helper so that you're not having to C&P comments all over
> the shop) explaining why you're creating multiple BootSourceCaches and what
> they'll be used for.

I've been staring at the code for so long that it didn't look that bad any
more. I guess I'll try for a helper - I ran out of brain juice on this one.

Thanks for reviewing!

« Back to merge proposal