Code review comment for lp:~michael.nelson/launchpad/496862-ppa-installable-binaries

On Thu, Mar 18, 2010 at 9:24 AM, Michael Nelson
<email address hidden> wrote:
> Michael Nelson has proposed merging lp:~michael.nelson/launchpad/496862-ppa-installable-binaries into lp:launchpad/devel.
>
> Requested reviews:
>  Canonical Launchpad Engineering (launchpad)
>
>
> This branch is the first of two to address bug 496862.
>
> It adds an ArchiveArchFactory to create a vocabulary dynamically depending on the binaries published in the archive.
>
> The ArchiveSourcePackageListViewBase class is refactored to ArchivePackagesViewBase so that it can be re-used for both source and binary packages.
>
> Finally, the ArchiveView (for the PPA index page) is updated to present binary packages.
>
> The next branch will switch the PPA index ui to present the binary packages.
>
> To test:
> bin/test -vv -t archive-views.txt -t archive.txt -t TestArchiveView
>

Good patch. I like the tests and the renames.

Some minor issues with method names and docstring formatting and a
couple of questions.

jml

> --
> https://code.launchpad.net/~michael.nelson/launchpad/496862-ppa-installable-binaries/+merge/21623
> Your team Launchpad code reviewers from Canonical is subscribed to branch lp:launchpad/devel.
>
> === modified file 'lib/lp/soyuz/browser/archive.py'
> --- lib/lp/soyuz/browser/archive.py     2010-03-10 12:50:18 +0000
> +++ lib/lp/soyuz/browser/archive.py     2010-03-18 09:24:24 +0000
> @@ -589,6 +589,26 @@
>         return SimpleVocabulary(series_terms)
>
>
> +class ArchiveArchVocabularyFactory:
> +    """A factory for generating vocabularies of an archive's arches in
> +    a given distroseries."""
> +
> +    implements(IContextSourceBinder)
> +
> +    def __call__(self, context):
> +        """Return a vocabulary created dynamically from the context archive.
> +
> +        :param context: The archive used to generating vocabulary.
> +        """
> +        arch_terms = []
> +        for arch in context.arches_with_binaries:
> +            term = SimpleTerm(
> +                arch, token=arch.architecturetag,
> +                title=arch.displayname)
> +            arch_terms.append(term)
> +        return SimpleVocabulary(arch_terms)
> +

Why is this a class and not a function? Is the implements() bit really
important?

> === added file 'lib/lp/soyuz/browser/tests/test_archive_view.py'
> --- lib/lp/soyuz/browser/tests/test_archive_view.py     1970-01-01 00:00:00 +0000
> +++ lib/lp/soyuz/browser/tests/test_archive_view.py     2010-03-18 09:24:24 +0000
> @@ -0,0 +1,59 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test the main PPA (user) view functionality."""
> +
> +__metaclass__ = type
> +
> +import unittest
> +
> +from canonical.launchpad.ftests import login
> +from canonical.testing import LaunchpadFunctionalLayer
> +from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.views import create_initialized_view
> +
> +
> +class TestArchiveView(TestCaseWithFactory):
> +
> +    layer = LaunchpadFunctionalLayer
> +

You need the librarian?

> +    def setUp(self):
> +        """Create a ppa with some publishings for different arches
> +        and in different distroseries."""

Closing triple quotes on newline please.

> +    def make_binaries_readable(self, binaries):

Methods should be camelCase :( I hate that rule.

> +    def test_batched_packages(self):

test_batchedPackages :(

> +    def test_batched_packages_filter_arch(self):

testBatchedPackagesFilterArch

> === modified file 'lib/lp/soyuz/interfaces/archive.py'
> --- lib/lp/soyuz/interfaces/archive.py  2010-03-17 16:45:38 +0000
> +++ lib/lp/soyuz/interfaces/archive.py  2010-03-18 09:24:24 +0000
> @@ -47,6 +47,7 @@
>  from canonical.launchpad.interfaces.launchpad import IPrivacy
>  from lp.registry.interfaces.role import IHasOwner
>  from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
> +from lp.soyuz.interfaces.distroarchseries import IDistroArchSeries
>  from lp.registry.interfaces.gpg import IGPGKey
>  from lp.registry.interfaces.person import IPerson
>  from canonical.launchpad.validators.name import name_validator
> @@ -223,6 +224,12 @@
>
>     series_with_sources = Attribute(
>         "DistroSeries to which this archive has published sources")
> +    arches_with_binaries = CollectionField(
> +            title=_(
> +                "DistroArchSeries to which this archive has published "
> +                "binaries."),
> +            value_type=Reference(schema=IDistroArchSeries),
> +            readonly=True)

Should this be exported for the webservice?

jml