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