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

Revision history for this message
Aaron Bentley (abentley) wrote :

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.

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).

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 thought distroseries were generally ordered by version or codename. Why is DistroSeriesDerivationVocabularyFactory using creation date instead?

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.

review: Needs Information

« Back to merge proposal