Code review comment for lp:~michael.nelson/launchpad/429318-copy-archives-timeout

 review needs-fixing

Thanks for this fix Michael. I have the usual comments in-line.

On Wednesday 16 September 2009 16:55:13 Michael Nelson wrote:
> I'm not sure if selecting the SourcePackageReleases is the right thing -
> I'm only using them to count etc. I could also use
> SourcePackagePublishingHistory - but (1) it would be an extra join, and
> (2) there are already methods on IArchive for this (that do not enable
> me to do what I need here.)

I think it's fine - in fact the old way of doing it was pretty inefficient
even without the prejoin problem.

> I've had to use removeSecurityProxy to be able to get to the difference
> method of ResultSet - this is because the interface at
> zope.storm.interfaces.IResultSet does not include difference, or
> _find_spec (which is accessed by the difference method).
>
> I'll create a bug for this and XXX it.

Ok, thanks.

>+ def getSourcePackageReleases(buildstatus=None):

Can you make this build_status please.

>+ """Return the releases for this archive.
>+
>+ :param buildstatus: If specified, only the distinct releases with
>+ builds in the specified build status will be returned.
>+ :return: A `ResultSet` of distinct `SourcePackageReleases` for this
>+ archive.
>+ """
>

>+ def getSourcePackageReleases(self, buildstatus=None):

And here of course.

>+ """See `IArchive`."""
>+ store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)

Is using the main store deliberate? I would have thought
 store = Store.of(self)
should suffice?

>+
>+ extra_exprs = []
>+ if buildstatus is not None:
>+ extra_exprs.append(Build.buildstate == buildstatus)
>+
>+ result_set = store.find(SourcePackageRelease,

PEP-8 - line feed after the open paren. please.

>+ Build.sourcepackagereleaseID == SourcePackageRelease.id,
>+ Build.archive == self,
>+ *extra_exprs)
>+
>+ result_set.config(distinct=True).order_by(SourcePackageRelease.id)
>+ return result_set
>
> class ArchiveSet:
> implements(IArchiveSet)
>
>=== modified file 'lib/lp/soyuz/tests/test_archive.py'
>--- lib/lp/soyuz/tests/test_archive.py 2009-07-27 14:45:01 +0000
>+++ lib/lp/soyuz/tests/test_archive.py 2009-09-16 15:22:15 +0000
>@@ -14,6 +14,7 @@
> from lp.registry.interfaces.distribution import IDistributionSet
> from lp.soyuz.interfaces.archive import IArchiveSet
> from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
>+from lp.soyuz.interfaces.build import BuildStatus
> from lp.soyuz.interfaces.publishing import PackagePublishingStatus
> from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> from lp.testing import TestCaseWithFactory
>@@ -243,5 +244,54 @@
> [u'1.0', u'0.5'], versions,
> "The latest version was not first.")
>
>+
>+class TestGetSourcePackageReleases(TestCaseWithFactory):
>+
>+ layer = LaunchpadZopelessLayer
>+
>+ def setUp(self):
>+ super(TestGetSourcePackageReleases, self).setUp()
>+ self.publisher = SoyuzTestPublisher()
>+ self.publisher.prepareBreezyAutotest()
>+
>+ # Create an archive with some published binaries.
>+ self.archive = self.factory.makeArchive()
>+ binaries_foo = self.publisher.getPubBinaries(
>+ archive=self.archive, binaryname="foo-bin")
>+ binaries_bar = self.publisher.getPubBinaries(
>+ archive=self.archive, binaryname="bar-bin")
>+
>+ # Collect the builds for reference.
>+ self.builds_foo = [
>+ binary.binarypackagerelease.build for binary in binaries_foo]
>+ self.builds_bar = [
>+ binary.binarypackagerelease.build for binary in binaries_bar]
>+
>+ # Collect the source package releases for reference.
>+ self.sourcepackagereleases = [
>+ self.builds_foo[0].sourcepackagerelease,
>+ self.builds_bar[0].sourcepackagerelease,
>+ ]
>+
>+ def test_getSourcePackageReleases_with_no_params(self):
>+ # With no params all source package releases are returned.
>+ sprs = self.archive.getSourcePackageReleases()
>+
>+ self.assertContentEqual(self.sourcepackagereleases, sprs)
>+
>+ def test_getSourcePackageReleases_with_buildstatus(self):
>+ # Results are filtered by the specified buildstatus.
>+
>+ # Set the builds for one of the sprs to needs build.
>+ for build in self.builds_foo:
>+ build.buildstate = BuildStatus.NEEDSBUILD
>+
>+ result = self.archive.getSourcePackageReleases(
>+ buildstatus=BuildStatus.NEEDSBUILD)
>+
>+ self.failUnlessEqual(1, result.count())
>+ self.failUnlessEqual(
>+ self.sourcepackagereleases[0], result[0])
>+
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)

Nice test.

I wonder if it should be in archive.txt though? At the very least, we need
*something* in archive.txt just to show the method exists.

Having said that, archive.txt is massive and more testing in that file is not
going to help matters. We could consider splitting that file up into more
logical documentation units.

Let's talk about it tomorrow.