Code review comment for lp:~allenap/launchpad/dd-initseries-bug-727105-derivation-vocab

Hi Aaron,

Thanks for looking at this. In answering your questions below I
discovered a situation I had not tested. I've remedied that, and also
managed to make the code a bit tighter, requiring fewer queries. It
seems I wasn't thinking very well while coding this branch.

> I don't know why you want to adapt a ProductSeries to a Product or a
> DistroSeries to a Distribution. It seems weird to me. I don't find any
> explanation of that in your cover letter. It would be helpful if you used our
> standard review template-- I'd expect to find this in the "implementation
> details" section.

It's a convenient way of getting a IDistribution context from anything
that wants to register for it. Having said that, I think this
vocabulary is only ever needed in an IDistroSeries context, so I
/could/ remove the adaption in the vocabulary and thus the adapters.

Also, note I didn't actually write the adapters; the code already
existed and I just registered it. Not sure what it was doing there to
be honest, but it suited my needs (and I did productseries_to_product
for completeness).

Something that factors into my reasoning is my (mild) dislike of:

    if IOneInterface.providedBy(an_argument):
        do_something()
    elif IOtherInterface.providedBy(an_argument):
        do_something_else()
    ...

when we have a component system all ready and running, although I do
admit that it can be expedient to just do things this way.

> With the adapters you've registered, wouldn't
> IDistribution.providedBy(distro_series) return True? If so, I think the tests
> are not testing whether the distroseries has been converted to a distribution.
> Instead of IDistribution.providedBy(distribution), you could do
> assertEqual(distribution, distro_series.distribution).

IDistribution.providedBy(distro_series) is not true. At least, it
shouldn't be if I understand correctly!

I've added the tests you suggest, but maybe I'll end up killing the
adapters altogether.

> I think the comment on LaunchpadObjectFactory.__dir__ is too narrow. While it
> is helpful to interactive users, it can be used in other ways.

I've changed it to:

        """Enumerate the attributes and methods of the wrapped object factory.

        This is especially useful for interactive users."""

Is that the kind of thing you were thinking of?

> I thought distroseries were generally ordered by version or codename. Why is
> DistroSeriesDerivationVocabularyFactory using creation date instead?

I've ordered it the same as DistroSeriesVocabulary, which seems to be
used quite widely.

> Getting a list of distroseries by doing set(distribution) is also pretty
> wacky. I would have expected set(distribution) to fail, because distributions
> don't seem like they should be iterable.

Yeah, it does read a bit oddly. I'm not the author (I think...) of
Distribution.__iter__() so I can't say why that was decided. I've
spent far too long on this branch as it is to revisit that decision,
so do you mind if I leave it as it is?