Code review comment for lp:~stevenk/launchpad/dsd-base_source_pub-search-parent

-----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 for the corresponding versions.
> dervied_changelog = self.factory.makeChangelog(
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk194oUACgkQBT3oW1L17ihBFwCdHMN+k5KTZFyzF8rggRtmZzJV
XosAnjzucBVTeSW7b2MVoa27NCCUtgFd
=tbD4
-----END PGP SIGNATURE-----