launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/classy-packagecloner into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch cleans up the PackageCloner to use instance variables so that the internal methods don't need to keep copying arguments around, and cleans a few pieces of lint I noticed.
-- 
https://code.launchpad.net/~stevenk/launchpad/classy-packagecloner/+merge/36963
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/classy-packagecloner into lp:launchpad/devel.

=== modified file 'lib/lp/soyuz/interfaces/packagecloner.py'
--- lib/lp/soyuz/interfaces/packagecloner.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/interfaces/packagecloner.py	2010-09-29 09:31:14 +0000
@@ -8,8 +8,8 @@
 __metaclass__ = type
 
 __all__ = [
-    'IPackageCloner'
-    ]
+    'IPackageCloner',
+]
 
 from zope.interface import Interface
 
@@ -17,7 +17,9 @@
 class IPackageCloner(Interface):
     """Copies publishing history data across archives."""
 
-    def clonePackages(origin, destination, distroarchseries_list=None):
+    def clonePackages(
+        origin, destination, distroarchseries_list=None,
+        proc_familes=None):
         """Copies the source packages from origin to destination as
         well as the binary packages for the DistroArchSeries specified.
 

=== modified file 'lib/lp/soyuz/model/packagecloner.py'
--- lib/lp/soyuz/model/packagecloner.py	2010-08-24 15:29:01 +0000
+++ lib/lp/soyuz/model/packagecloner.py	2010-09-29 09:31:14 +0000
@@ -7,7 +7,6 @@
 
 __all__ = [
     'PackageCloner',
-    'clone_packages',
     ]
 
 
@@ -31,36 +30,15 @@
 from lp.soyuz.interfaces.packagecloner import IPackageCloner
 
 
-def clone_packages(origin, destination, distroarchseries_list=None):
-    """Copies packages from origin to destination package location.
-
-    Binary packages are only copied for the `DistroArchSeries` pairs
-    specified.
-
-    This function is meant to simplify the utilization of the package
-    cloning functionality.
-
-    @type origin: PackageLocation
-    @param origin: the location from which packages are to be copied.
-    @type destination: PackageLocation
-    @param destination: the location to which the data is to be copied.
-    @type distroarchseries_list: list of pairs of (origin, destination)
-        distroarchseries instances.
-    @param distroarchseries_list: the binary packages will be copied
-        for the distroarchseries pairs specified (if any).
-    """
-    pkg_cloner = getUtility(IPackageCloner)
-    pkg_cloner.clonePackages(origin, destination, distroarchseries_list)
-
-
 class PackageCloner:
     """Used for copying of various publishing history data across archives.
     """
 
     implements(IPackageCloner)
 
-    def clonePackages(self, origin, destination, distroarchseries_list=None,
-                      proc_families=None):
+    def clonePackages(
+        self, origin, destination, distroarchseries_list=None,
+        proc_families=None):
         """Copies packages from origin to destination package location.
 
         Binary packages are only copied for the `DistroArchSeries` pairs
@@ -74,35 +52,38 @@
             distroarchseries instances.
         @param distroarchseries_list: the binary packages will be copied
             for the distroarchseries pairs specified (if any).
-        @param the processor families to create builds for.
+        @param proc_families: the processor families to create builds for.
         """
+        self.origin = origin
+        self.destination = destination
+        self.distroarchseries_list = distroarchseries_list
+        if proc_families is None:
+            self.proc_families = []
+        else:
+            self.proc_families = proc_families
+
         # First clone the source packages.
-        self._clone_source_packages(origin, destination)
+        self._clone_source_packages()
+        #self._clone_source_packages(origin, destination)
 
         # Are we also supposed to clone binary packages from origin to
         # destination distroarchseries pairs?
-        if distroarchseries_list is not None:
-            for (origin_das, destination_das) in distroarchseries_list:
-                self._clone_binary_packages(
-                    origin, destination, origin_das, destination_das)
-
-        if proc_families is None:
-            proc_families = []
-
-        self._create_missing_builds(
-            destination.distroseries, destination.archive, proc_families)
-
-    def _create_missing_builds(self, distroseries, archive, proc_families):
+        if self.distroarchseries_list is not None:
+            for (origin_das, destination_das) in self.distroarchseries_list:
+                self._clone_source_packages(origin_das, destination_das)
+                #self._clone_binary_packages(
+                #    origin, destination, origin_das, destination_das)
+
+        self._create_missing_builds()
+
+    def _create_missing_builds(self):
         """Create builds for all cloned source packages.
-
-        :param distroseries: the distro series for which to create builds.
-        :param archive: the archive for which to create builds.
-        :param proc_families: the list of processor families for
-            which to create builds.
         """
         # Avoid circular imports.
         from lp.soyuz.interfaces.publishing import active_publishing_status
 
+        distroseries = self.destination.distroseries
+
         # Listify the architectures to avoid hitting this MultipleJoin
         # multiple times.
         architectures = list(distroseries.architectures)
@@ -110,7 +91,7 @@
         # Filter the list of DistroArchSeries so that only the ones
         # specified in proc_families remain
         architectures = [architecture for architecture in architectures
-             if architecture.processorfamily in proc_families]
+             if architecture.processorfamily in self.proc_families]
 
         if len(architectures) == 0:
             return
@@ -118,28 +99,17 @@
         # Both, PENDING and PUBLISHED sources will be considered for
         # as PUBLISHED. It's part of the assumptions made in:
         # https://launchpad.net/soyuz/+spec/build-unpublished-source
-        sources_published = archive.getPublishedSources(
+        sources_published = self.destination.archive.getPublishedSources(
             distroseries=distroseries, status=active_publishing_status)
 
-        def get_spn(pub):
-            """Return the source package name for a publishing record."""
-            return pub.sourcepackagerelease.sourcepackagename.name
-
         for pubrec in sources_published:
             pubrec.createMissingBuilds(architectures_available=architectures)
             # Commit to avoid MemoryError: bug 304459
             transaction.commit()
 
-    def _clone_binary_packages(self, origin, destination, origin_das,
-                              destination_das):
+    def _clone_binary_packages(self, origin_das, destination_das):
         """Copy binary publishing data from origin to destination.
 
-        @type origin: PackageLocation
-        @param origin: the location from which binary publishing
-            records are to be copied.
-        @type destination: PackageLocation
-        @param destination: the location to which the data is
-            to be copied.
         @type origin_das: DistroArchSeries
         @param origin_das: the DistroArchSeries from which to copy
             binary packages
@@ -162,11 +132,11 @@
             AND
                 bpph.pocket = %s and bpph.archive = %s
             ''' % sqlvalues(
-                destination_das, destination.archive, UTC_NOW, UTC_NOW,
-                destination.pocket, origin_das,
+                destination_das, self.destination.archive, UTC_NOW,
+                UTC_NOW, self.destination.pocket, origin_das,
                 PackagePublishingStatus.PENDING,
                 PackagePublishingStatus.PUBLISHED,
-                origin.pocket, origin.archive))
+                self.origin.pocket, self.origin.archive))
 
     def mergeCopy(self, origin, destination):
         """Please see `IPackageCloner`."""
@@ -211,12 +181,11 @@
             """Extract the processor family from an `IArchiveArch`."""
             return removeSecurityProxy(archivearch).processorfamily
 
-        proc_families = [
+        self.proc_families = [
             get_family(archivearch) for archivearch
             in getUtility(IArchiveArchSet).getByArchive(destination.archive)]
 
-        self._create_missing_builds(
-            destination.distroseries, destination.archive, proc_families)
+        self._create_missing_builds()
 
     def _compute_packageset_delta(self, origin):
         """Given a source/target archive find obsolete or missing packages.
@@ -245,7 +214,8 @@
                 secsrc.sourcepackagerelease = spr.id AND
                 spr.sourcepackagename = spn.id AND
                 spn.name = mcd.sourcepackagename AND
-                debversion_sort_key(spr.version) > debversion_sort_key(mcd.t_version)
+                debversion_sort_key(spr.version) >
+                debversion_sort_key(mcd.t_version)
         """ % sqlvalues(
                 origin.archive,
                 PackagePublishingStatus.PENDING,
@@ -359,15 +329,8 @@
                 " AND secsrc.component = %s" % quote(destination.component))
         store.execute(pop_query)
 
-    def _clone_source_packages(self, origin, destination):
+    def _clone_source_packages(self):
         """Copy source publishing data from origin to destination.
-
-        @type origin: PackageLocation
-        @param origin: the location from which source publishing
-            records are to be copied.
-        @type destination: PackageLocation
-        @param destination: the location to which the data is
-            to be copied.
         """
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
         query = '''
@@ -382,13 +345,14 @@
             WHERE spph.distroseries = %s AND spph.status in (%s, %s) AND
                   spph.pocket = %s and spph.archive = %s
             ''' % sqlvalues(
-                destination.distroseries, destination.archive, UTC_NOW,
-                UTC_NOW, destination.pocket, origin.distroseries,
+                self.destination.distroseries, self.destination.archive,
+                UTC_NOW, UTC_NOW, self.destination.pocket,
+                self.origin.distroseries,
                 PackagePublishingStatus.PENDING,
                 PackagePublishingStatus.PUBLISHED,
-                origin.pocket, origin.archive)
+                self.origin.pocket, self.origin.archive)
 
-        if origin.packagesets:
+        if self.origin.packagesets:
             query += '''AND spph.sourcepackagerelease IN
                             (SELECT spr.id
                              FROM SourcePackageRelease AS spr,
@@ -398,10 +362,12 @@
                                     = pss.sourcepackagename
                              AND pss.packageset = fpsi.child
                              AND fpsi.parent in %s)
-                     ''' % sqlvalues([p.id for p in origin.packagesets])
+                     ''' % sqlvalues([p.id for p in
+                        self.origin.packagesets])
 
-        if origin.component:
-            query += "and spph.component = %s" % sqlvalues(origin.component)
+        if self.origin.component:
+            query += "and spph.component = %s" % sqlvalues(
+                self.origin.component)
 
         store.execute(query)