Merge lp:~stevenk/launchpad/dsd-base_source_pub-search-parent into lp:launchpad

Proposed by Steve Kowalik on 2011-03-14
Status: Merged
Approved by: Steve Kowalik on 2011-03-14
Approved revision: no longer in the source branch.
Merged at revision: 12592
Proposed branch: lp:~stevenk/launchpad/dsd-base_source_pub-search-parent
Merge into: lp:launchpad
Diff against target: 113 lines (+48/-11)
3 files modified
lib/lp/registry/interfaces/distroseriesdifference.py (+2/-1)
lib/lp/registry/model/distroseriesdifference.py (+11/-8)
lib/lp/registry/tests/test_distroseriesdifference.py (+35/-2)
To merge this branch: bzr merge lp:~stevenk/launchpad/dsd-base_source_pub-search-parent
Reviewer Review Type Date Requested Status
Henning Eggers (community) code 2011-03-14 Approve on 2011-03-14
Review via email: mp+53194@code.launchpad.net

Commit message

[r=henninge][bug=734719] DSD.base_source_pub now looks in the parent series first, and then the child distroseries.

Description of the change

DSD.base_source_pub should look in the parent series first, since that is most likely to contain the base version publication in the real world.

To post a comment you must log in.
Robert Collins (lifeless) wrote :

This does two queries always - the count() (expensive) and then a first.

Better to just do a first:
result = pubs.first()
if result is None:
    result = ... other side query ... .first()
return result

Henning Eggers (henninge) wrote :
Download full text (3.5 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Steven,
thanks for the quick chat and explanation. I should have looked at more
context but I had hoped to do it quickly.

 review approve code

This looks fine to me, only the two issues mentioned on mumble. Please have a
look below.

Cheers,
Henning

Am 14.03.2011 06:23, schrieb Steve Kowalik:
> === modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
> === modified file 'lib/lp/registry/model/distroseriesdifference.py'
> === modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
> --- lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-10 11:11:50 +0000
> +++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-14 05:22:57 +0000
> @@ -474,7 +474,8 @@
[...]

> @@ -515,6 +517,36 @@
> self.assertEqual('1.0', ds_diff.base_version)
> self.assertIs(None, ds_diff.base_source_pub)
>
> + def test_base_source_pub_only_in_child(self):
> + # If the base version is only published in the child distroseries,
> + # the base source publication is still located and returned.
> + derived_changelog = self.factory.makeChangelog(
> + versions=['1.0', '1.2'])
> + parent_changelog = self.factory.makeChangelog(
> + versions=['1.0', '1.3'])
> + transaction.commit() # Yay, librarian.
> +
> + ds_diff = self.factory.makeDistroSeriesDifference(versions={
> + 'derived': '1.2',
> + 'parent': '1.3',
> + },
> + changelogs={
> + 'derived': derived_changelog,
> + 'parent': parent_changelog,
> + })

This is formatted a bit inconsistently that makes it harder to read. This
would be better, I think.

        ds_diff = self.factory.makeDistroSeriesDifference(
            versions={
                'derived': '1.2',
                'parent': '1.3',
                },
            changelogs={
                'derived': derived_changelog,
                'parent': parent_changelog,
                })

I *think* it is python style to place closing braces on the same level as the
content?

> +
> + # Passing in a base version to makeDistroSeriesDifference() creates
> + # it in both distroseries, so we need to do it ourselves.

So, the comment would be much clearer to me like this but maybe that's too
verbose?

# We do not pass in a 'base' version to makeDistroSeriesDifference()
# because it would be created in both distroseries and we don't want
# that. We rather create it ourselves in just the child series.

> + spr = self.factory.makeSourcePackageRelease(
> + sourcepackagename=ds_diff.source_package_name, version='1.0')
> + self.factory.makeSourcePackagePublishingHistory(
> + distroseries=ds_diff.derived_series, sourcepackagerelease=spr,
> + status=PackagePublishingStatus.SUPERSEDED)
> + ds_diff.update()
> + self.assertEqual('1.0', ds_diff.base_version)
> + self.assertEqual(
> + ds_diff.derived_series, ds_diff.base_source_pub.distroseries)
> +
> def test_requestPackageDiffs(self):
> # IPackageDiffs are created f...

Read more...

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
2--- lib/lp/registry/interfaces/distroseriesdifference.py 2010-10-07 14:59:21 +0000
3+++ lib/lp/registry/interfaces/distroseriesdifference.py 2011-03-14 09:44:28 +0000
4@@ -132,7 +132,8 @@
5 ISourcePackagePublishingHistory,
6 title=_("Base source pub"), readonly=True,
7 description=_(
8- "The common base version published in the derived series."))
9+ "The common base version published in the parent or the "
10+ "derived series."))
11
12 owner = Reference(
13 IPerson, title=_("Owning team of the derived series"), readonly=True,
14
15=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
16--- lib/lp/registry/model/distroseriesdifference.py 2011-03-10 04:00:08 +0000
17+++ lib/lp/registry/model/distroseriesdifference.py 2011-03-14 09:44:28 +0000
18@@ -29,7 +29,6 @@
19 IMasterStore,
20 IStore,
21 )
22-from lp.archivepublisher.debversion import Version
23 from lp.registry.enum import (
24 DistroSeriesDifferenceStatus,
25 DistroSeriesDifferenceType,
26@@ -154,14 +153,18 @@
27 def base_source_pub(self):
28 """See `IDistroSeriesDifference`."""
29 if self.base_version is not None:
30- pubs = self.derived_series.main_archive.getPublishedSources(
31+ parent = self.derived_series.parent_series
32+ result = parent.main_archive.getPublishedSources(
33 name=self.source_package_name.name,
34- version=self.base_version,
35- distroseries=self.derived_series)
36- # We know there is a base version published in the distroseries'
37- # main archive.
38- return pubs.first()
39-
40+ version=self.base_version, distroseries=parent).first()
41+ if result is None:
42+ # If the base version isn't in the parent, it may be
43+ # published in the child distroseries.
44+ child = self.derived_series
45+ result = child.main_archive.getPublishedSources(
46+ name=self.source_package_name.name,
47+ version=self.base_version, distroseries=child).first()
48+ return result
49 return None
50
51 @property
52
53=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
54--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-10 11:11:50 +0000
55+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-03-14 09:44:28 +0000
56@@ -474,7 +474,8 @@
57 self.assertIs(None, ds_diff.base_source_pub)
58
59 def test_base_source_pub(self):
60- # The publishing in the derived series with base_version is returned.
61+ # The publication in the parent series with the base version is
62+ # returned.
63 derived_changelog = self.factory.makeChangelog(
64 versions=['1.0', '1.2'])
65 parent_changelog = self.factory.makeChangelog(
66@@ -493,7 +494,8 @@
67
68 base_pub = ds_diff.base_source_pub
69 self.assertEqual('1.0', base_pub.source_package_version)
70- self.assertEqual(ds_diff.derived_series, base_pub.distroseries)
71+ self.assertEqual(
72+ ds_diff.derived_series.parent_series, base_pub.distroseries)
73
74 def test_base_source_pub_not_published(self):
75 # If the base version isn't published, the base version is
76@@ -515,6 +517,37 @@
77 self.assertEqual('1.0', ds_diff.base_version)
78 self.assertIs(None, ds_diff.base_source_pub)
79
80+ def test_base_source_pub_only_in_child(self):
81+ # If the base version is only published in the child distroseries,
82+ # the base source publication is still located and returned.
83+ derived_changelog = self.factory.makeChangelog(
84+ versions=['1.0', '1.2'])
85+ parent_changelog = self.factory.makeChangelog(
86+ versions=['1.0', '1.3'])
87+ transaction.commit() # Yay, librarian.
88+
89+ ds_diff = self.factory.makeDistroSeriesDifference(
90+ versions={
91+ 'derived': '1.2',
92+ 'parent': '1.3',
93+ },
94+ changelogs={
95+ 'derived': derived_changelog,
96+ 'parent': parent_changelog,
97+ })
98+
99+ # Passing in a base version to makeDistroSeriesDifference() creates
100+ # it in both distroseries, which we don't want, so we need to do it
101+ # ourselves.
102+ spr = self.factory.makeSourcePackageRelease(
103+ sourcepackagename=ds_diff.source_package_name, version='1.0')
104+ self.factory.makeSourcePackagePublishingHistory(
105+ distroseries=ds_diff.derived_series, sourcepackagerelease=spr,
106+ status=PackagePublishingStatus.SUPERSEDED)
107+ self.assertEqual('1.0', ds_diff.base_version)
108+ self.assertEqual(
109+ ds_diff.derived_series, ds_diff.base_source_pub.distroseries)
110+
111 def test_requestPackageDiffs(self):
112 # IPackageDiffs are created for the corresponding versions.
113 dervied_changelog = self.factory.makeChangelog(