Launchpad itself

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

  1. 496862-ppa-installable-binaries
  2. Merge into devel
Revision history for this message
Michael Nelson (michael.nelson) wrote :

On Thu, Mar 18, 2010 at 12:03 PM, Jonathan Lange <email address hidden> wrote:
> On Thu, Mar 18, 2010 at 9:24 AM, Michael Nelson
 ...
>> === 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?
>

Yep, but I used the zope.interface.directlyProvides as you mentioned,
and it worked well :)

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

Yes, publishing test data with the SoyuzTestPublisher adds librarian
files for the tests.

>
>> +    def setUp(self):
>> +        """Create a ppa with some publishings for different arches
>> +        and in different distroseries."""
>
> Closing triple quotes on newline please.

Done.

>
>> +    def make_binaries_readable(self, binaries):
>
> Methods should be camelCase :( I hate that rule.

Done.

>
>> +    def test_batched_packages(self):
>
> test_batchedPackages :(

Not done, as your subsequent email.

>
>> +    def test_batched_packages_filter_arch(self):
>
> testBatchedPackagesFilterArch

And again.

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

If there's a need later, right now it's only needed by the browser
class, and it's not something generally useful.

Thanks!

>
> jml
> --
> https://code.launchpad.net/~michael.nelson/launchpad/496862-ppa-installable-binaries/+merge/21623
> You are the owner of lp:~michael.nelson/launchpad/496862-ppa-installable-binaries.
>

changes_jml.diff
1=== modified file 'lib/lp/soyuz/browser/archive.py'
2--- lib/lp/soyuz/browser/archive.py 2010-03-18 09:08:28 +0000
3+++ lib/lp/soyuz/browser/archive.py 2010-03-18 17:01:59 +0000
4@@ -33,7 +33,7 @@
5 from zope.app.form.browser import TextAreaWidget
6 from zope.component import getUtility
7 from zope.formlib import form
8-from zope.interface import implements, Interface
9+from zope.interface import directlyProvides, implements, Interface
10 from zope.security.proxy import removeSecurityProxy
11 from zope.schema import Choice, List, TextLine
12 from zope.schema.interfaces import IContextSourceBinder
13@@ -568,45 +568,22 @@
14 return list(copy_requests)
15
16
17-class ArchiveSeriesVocabularyFactory:
18- """A factory for generating vocabularies of an archive's series."""
19-
20- implements(IContextSourceBinder)
21-
22- def __call__(self, context):
23- """Return a vocabulary created dynamically from the context archive.
24-
25- :param context: The context used to generate the vocabulary. This
26- is passed automatically by the zope machinery. Therefore
27- this factory can only be used in a class where the context is
28- an IArchive.
29- """
30- series_terms = []
31- for distroseries in context.series_with_sources:
32- series_terms.append(
33- SimpleTerm(distroseries, token=distroseries.name,
34- title=distroseries.displayname))
35- return SimpleVocabulary(series_terms)
36-
37-
38-class ArchiveArchVocabularyFactory:
39- """A factory for generating vocabularies of an archive's arches in
40- a given distroseries."""
41-
42- implements(IContextSourceBinder)
43-
44- def __call__(self, context):
45- """Return a vocabulary created dynamically from the context archive.
46-
47- :param context: The archive used to generating vocabulary.
48- """
49- arch_terms = []
50- for arch in context.arches_with_binaries:
51- term = SimpleTerm(
52- arch, token=arch.architecturetag,
53- title=arch.displayname)
54- arch_terms.append(term)
55- return SimpleVocabulary(arch_terms)
56+def archive_series_vocabulary_factory(context):
57+ """Return a vocabulary created dynamically from the context archive.
58+
59+ :param context: The context used to generate the vocabulary. This
60+ is passed automatically by the zope machinery. Therefore
61+ this factory can only be used in a class where the context is
62+ an IArchive.
63+ """
64+ series_terms = []
65+ for distroseries in context.series_with_sources:
66+ series_terms.append(
67+ SimpleTerm(distroseries, token=distroseries.name,
68+ title=distroseries.displayname))
69+ return SimpleVocabulary(series_terms)
70+
71+directlyProvides(archive_series_vocabulary_factory, IContextSourceBinder)
72
73
74 class SeriesFilterWidget(LaunchpadDropdownWidget):
75@@ -625,7 +602,7 @@
76 title=_("Package name contains"), required=False)
77
78 series_filter = Choice(
79- source=ArchiveSeriesVocabularyFactory(), required=False)
80+ source=archive_series_vocabulary_factory, required=False)
81
82 status_filter = Choice(vocabulary=SimpleVocabulary((
83 SimpleTerm(active_publishing_status, 'published', 'Published'),
84@@ -633,10 +610,26 @@
85 )), required=False)
86
87
88+def archive_arch_vocabulary_factory(context):
89+ """Return a vocabulary created dynamically from the context archive.
90+
91+ :param context: The archive used to generating vocabulary.
92+ """
93+ arch_terms = []
94+ for arch in context.arches_with_binaries:
95+ term = SimpleTerm(
96+ arch, token=arch.architecturetag,
97+ title=arch.displayname)
98+ arch_terms.append(term)
99+ return SimpleVocabulary(arch_terms)
100+
101+directlyProvides(archive_arch_vocabulary_factory, IContextSourceBinder)
102+
103+
104 class IArchiveBinaryPackageFilter(IPPAPackageFilter):
105 """The interface used for filtering binary packages."""
106 arch_filter = Choice(
107- source=ArchiveArchVocabularyFactory(), required=False)
108+ source=archive_arch_vocabulary_factory, required=False)
109
110
111 class ArchivePackagesViewBase(ArchiveViewBase, LaunchpadFormView):
112
113=== modified file 'lib/lp/soyuz/browser/tests/test_archive_view.py'
114--- lib/lp/soyuz/browser/tests/test_archive_view.py 2010-03-18 09:06:00 +0000
115+++ lib/lp/soyuz/browser/tests/test_archive_view.py 2010-03-18 17:14:42 +0000
116@@ -29,7 +29,7 @@
117
118 test_publisher.getPubBinaries(archive=self.ppa)
119
120- def make_binaries_readable(self, binaries):
121+ def makeBinariesReadable(self, binaries):
122 """Return a string representation of the binaries."""
123 return ", ".join([
124 "%s in %s" % (
125@@ -45,7 +45,7 @@
126 self.assertEqual(
127 "foo-bin in ubuntutest Breezy Badger Autotest i386, "
128 "foo-bin in ubuntutest Breezy Badger Autotest hppa",
129- self.make_binaries_readable(view.batched_packages))
130+ self.makeBinariesReadable(view.batched_packages))
131
132 def test_batched_packages_filter_arch(self):
133 view = create_initialized_view(
134@@ -53,7 +53,7 @@
135 query_string="field.arch_filter=i386")
136 self.assertEqual(
137 "foo-bin in ubuntutest Breezy Badger Autotest i386",
138- self.make_binaries_readable(view.batched_packages))
139+ self.makeBinariesReadable(view.batched_packages))
140
141 def test_suite():
142 return unittest.TestLoader().loadTestsFromName(__name__)

« Back to merge proposal