Code review comment for lp:~allenap/launchpad/dd-initseries-bug-727105-derivation-vocab
- dd-initseries-bug-727105-derivation-vocab
- Merge into devel
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
| 1 | === modified file 'lib/lp/registry/tests/test_distroseries_vocabularies.py' |
| 2 | --- lib/lp/registry/tests/test_distroseries_vocabularies.py 2011-03-11 17:29:36 +0000 |
| 3 | +++ lib/lp/registry/tests/test_distroseries_vocabularies.py 2011-03-14 14:58:34 +0000 |
| 4 | @@ -51,28 +51,20 @@ |
| 5 | def test_interfaces(self): |
| 6 | # DistroSeriesDerivationVocabularyFactory instances provide |
| 7 | # IVocabulary and IVocabularyTokenized. |
| 8 | - distribution = self.factory.makeDistribution() |
| 9 | - vocabulary = self.vocabulary_factory(distribution) |
| 10 | + distroseries = self.factory.makeDistroSeries() |
| 11 | + vocabulary = self.vocabulary_factory(distroseries) |
| 12 | self.assertProvides(vocabulary, IVocabulary) |
| 13 | self.assertProvides(vocabulary, IVocabularyTokenized) |
| 14 | |
| 15 | - def test_distribution_without_series(self): |
| 16 | - # Given a distribution without any series, the vocabulary factory |
| 17 | - # returns a vocabulary for all distroseries in all distributions. |
| 18 | - distribution = self.factory.makeDistribution() |
| 19 | - vocabulary = self.vocabulary_factory(distribution) |
| 20 | - expected_distroseries = set(self.all_distroseries) |
| 21 | - observed_distroseries = set(term.value for term in vocabulary) |
| 22 | - self.assertEqual(expected_distroseries, observed_distroseries) |
| 23 | - |
| 24 | def test_distribution_with_non_derived_series(self): |
| 25 | # Given a distribution with series, none of which are derived, the |
| 26 | # vocabulary factory returns a vocabulary for all distroseries in all |
| 27 | # distributions *except* the given distribution. |
| 28 | distroseries = self.factory.makeDistroSeries() |
| 29 | - vocabulary = self.vocabulary_factory(distroseries.distribution) |
| 30 | + vocabulary = self.vocabulary_factory(distroseries) |
| 31 | expected_distroseries = ( |
| 32 | - set(self.all_distroseries) - set(distroseries.distribution)) |
| 33 | + set(self.all_distroseries).difference( |
| 34 | + distroseries.distribution.series)) |
| 35 | observed_distroseries = set(term.value for term in vocabulary) |
| 36 | self.assertEqual(expected_distroseries, observed_distroseries) |
| 37 | |
| 38 | @@ -86,8 +78,8 @@ |
| 39 | distribution=parent_distroseries.distribution) |
| 40 | distroseries = self.factory.makeDistroSeries( |
| 41 | parent_series=parent_distroseries) |
| 42 | - vocabulary = self.vocabulary_factory(distroseries.distribution) |
| 43 | - expected_distroseries = set(parent_distroseries.distribution) |
| 44 | + vocabulary = self.vocabulary_factory(distroseries) |
| 45 | + expected_distroseries = set(parent_distroseries.distribution.series) |
| 46 | observed_distroseries = set(term.value for term in vocabulary) |
| 47 | self.assertEqual(expected_distroseries, observed_distroseries) |
| 48 | |
| 49 | @@ -100,9 +92,10 @@ |
| 50 | distroseries = self.factory.makeDistroSeries( |
| 51 | distribution=parent_distroseries.distribution, |
| 52 | parent_series=parent_distroseries) |
| 53 | - vocabulary = self.vocabulary_factory(distroseries.distribution) |
| 54 | + vocabulary = self.vocabulary_factory(distroseries) |
| 55 | expected_distroseries = ( |
| 56 | - set(self.all_distroseries) - set(distroseries.distribution)) |
| 57 | + set(self.all_distroseries).difference( |
| 58 | + distroseries.distribution.series)) |
| 59 | observed_distroseries = set(term.value for term in vocabulary) |
| 60 | self.assertEqual(expected_distroseries, observed_distroseries) |
| 61 | |
| 62 | @@ -112,7 +105,8 @@ |
| 63 | distroseries = self.factory.makeDistroSeries() |
| 64 | vocabulary = self.vocabulary_factory(distroseries) |
| 65 | expected_distroseries = ( |
| 66 | - set(self.all_distroseries) - set(distroseries.distribution)) |
| 67 | + set(self.all_distroseries).difference( |
| 68 | + distroseries.distribution.series)) |
| 69 | observed_distroseries = set(term.value for term in vocabulary) |
| 70 | self.assertEqual(expected_distroseries, observed_distroseries) |
| 71 | |
| 72 | @@ -140,8 +134,9 @@ |
| 73 | removeSecurityProxy(bbb_series_newer).date_created = two_days_ago |
| 74 | |
| 75 | ccc = self.factory.makeDistribution(displayname="ccc") |
| 76 | + ccc_series = self.factory.makeDistroSeries(distribution=ccc) |
| 77 | |
| 78 | - vocabulary = self.vocabulary_factory(ccc) |
| 79 | + vocabulary = self.vocabulary_factory(ccc_series) |
| 80 | expected_distroseries = [ |
| 81 | aaa_series_newer, aaa_series_older, |
| 82 | bbb_series_newer, bbb_series_older] |
| 83 | @@ -154,27 +149,20 @@ |
| 84 | if series in expected_distroseries] |
| 85 | self.assertEqual(expected_distroseries, observed_distroseries) |
| 86 | |
| 87 | - def test_queries_for_distribution_without_series(self): |
| 88 | - for index in range(10): |
| 89 | - self.factory.makeDistroSeries() |
| 90 | - distribution = self.factory.makeDistribution() |
| 91 | - flush_database_caches() |
| 92 | - # Getting terms issues two queries: one for the distribution's |
| 93 | - # serieses and a second for all serieses. |
| 94 | - with StormStatementRecorder() as recorder: |
| 95 | - self.vocabulary_factory(distribution).terms |
| 96 | - self.assertThat(recorder, HasQueryCount(Equals(2))) |
| 97 | - |
| 98 | def test_queries_for_distribution_with_non_derived_series(self): |
| 99 | for index in range(10): |
| 100 | self.factory.makeDistroSeries() |
| 101 | distribution = self.factory.makeDistribution() |
| 102 | - self.factory.makeDistroSeries(distribution=distribution) |
| 103 | + distroseries = self.factory.makeDistroSeries( |
| 104 | + distribution=distribution) |
| 105 | flush_database_caches() |
| 106 | + # Reload distroseries and distribution; these will reasonably already |
| 107 | + # be loaded before using the vocabulary. |
| 108 | + distroseries.distribution |
| 109 | # Getting terms issues two queries: one to search for parent serieses |
| 110 | # (of which there are none) and a second for all serieses. |
| 111 | with StormStatementRecorder() as recorder: |
| 112 | - self.vocabulary_factory(distribution).terms |
| 113 | + self.vocabulary_factory(distroseries).terms |
| 114 | self.assertThat(recorder, HasQueryCount(Equals(2))) |
| 115 | |
| 116 | def test_queries_for_distribution_with_derived_series(self): |
| 117 | @@ -182,11 +170,14 @@ |
| 118 | self.factory.makeDistroSeries() |
| 119 | distribution = self.factory.makeDistribution() |
| 120 | parent_distroseries = self.factory.makeDistroSeries() |
| 121 | - self.factory.makeDistroSeries( |
| 122 | + distroseries = self.factory.makeDistroSeries( |
| 123 | parent_series=parent_distroseries, |
| 124 | distribution=distribution) |
| 125 | flush_database_caches() |
| 126 | + # Reload distroseries and distribution; these will reasonably already |
| 127 | + # be loaded before using the vocabulary. |
| 128 | + distroseries.distribution |
| 129 | # Getting terms issues one query to find parent serieses. |
| 130 | with StormStatementRecorder() as recorder: |
| 131 | - self.vocabulary_factory(distribution).terms |
| 132 | + self.vocabulary_factory(distroseries).terms |
| 133 | self.assertThat(recorder, HasQueryCount(Equals(1))) |
| 134 | |
| 135 | === modified file 'lib/lp/registry/vocabularies.py' |
| 136 | --- lib/lp/registry/vocabularies.py 2011-03-11 17:29:36 +0000 |
| 137 | +++ lib/lp/registry/vocabularies.py 2011-03-14 14:36:36 +0000 |
| 138 | @@ -1500,7 +1500,8 @@ |
| 139 | |
| 140 | def __init__(self, context): |
| 141 | """See `IVocabularyFactory.__call__`.""" |
| 142 | - self.context = context |
| 143 | + assert IDistroSeries.providedBy(context) |
| 144 | + self.distribution = context.distribution |
| 145 | |
| 146 | def find_terms(self, *where): |
| 147 | """Return a `tuple` of terms matching the given criteria. |
| 148 | @@ -1525,19 +1526,18 @@ |
| 149 | |
| 150 | The order is the same as for `DistroSeriesVocabulary`. |
| 151 | """ |
| 152 | - distribution = IDistribution(self.context) |
| 153 | parent = ClassAlias(DistroSeries, "parent") |
| 154 | child = ClassAlias(DistroSeries, "child") |
| 155 | parent_distributions = Select( |
| 156 | parent.distributionID, And( |
| 157 | - parent.distributionID != distribution.id, |
| 158 | - child.distributionID == distribution.id, |
| 159 | + parent.distributionID != self.distribution.id, |
| 160 | + child.distributionID == self.distribution.id, |
| 161 | child.parent_seriesID == parent.id)) |
| 162 | terms = self.find_terms( |
| 163 | DistroSeries.distributionID.is_in(parent_distributions)) |
| 164 | if len(terms) == 0: |
| 165 | terms = self.find_terms( |
| 166 | - DistroSeries.distribution != distribution) |
| 167 | + DistroSeries.distribution != self.distribution) |
| 168 | return terms |
| 169 | |
| 170 | @cachedproperty |
> > It's a convenient way of getting a IDistribution context from anything
> > that wants to register for it.
>
> That's a definition of an adapter, but it doesn't explain the motivation
> for providing this adaptation.
I don't see the harm in doing it, but that's perhaps because I've not
really experienced a situation where this kind of thing has led to
problems later on.
>
> > 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.
>
> That would seem like a more straightforward way of doing things.
All the vocabulary needs is a distribution to function, yet it will
currently only be used in a distroseries context. It seems strange to
require a distroseries when I could just say "give me anything that
can be adapted to a distribution".
Anyway, I've made it work only for distroseries now.
>
> > Also, note I didn't actually write the adapters; the code already
> > existed and I just registered it.
>
> Instead of going "We haven't used it" and deleting it? :-)
I was wrong; that code is actually used as the basis for other
adapters.
>
> > Not sure what it was doing there to
> > be honest, but it suited my needs (and I did productseries_
> > for completeness).
> >
> > Something that factors into my reasoning is my (mild) dislike of:
> >
> > if IOneInterface.
> > do_something()
> > elif IOtherInterface
> > do_something_else()
>
> I share that dislike, but I don't see how that applies to this context.
If I don't use adapters but want to keep the ability to work in a
distribution or distroseries context I can't really see a way around
the IFoo.providedBy
Well, I could getattr(context, "distribution", context) but I suspect that
might an order of magnitude worse again :)
>
> This is a situation where you're passing in objects of the wrong type
> and adapting them to objects of the correct type. Why not just pass in
> objects of the correct type?
Because it's a named vocabulary. You get the context in which it is
used. I don't think there is an opportunity to select what gets
passed.
> Distribution.
> I recommend using that instead.
Done.
Thanks again!