launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/packageset-score-rename-finish into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1000570 in Launchpad itself: ""Packageset.score" is badly named"
  https://bugs.launchpad.net/launchpad/+bug/1000570

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/packageset-score-rename-finish/+merge/110046

== Summary ==

Complete the renaming of Packageset.score to Packageset.relative_build_score (bug 1000570).

== Proposed fix ==

Now that https://code.launchpad.net/~cjwatson/launchpad/db-packageset-score-rename/+merge/108336 has been landed and rolled out, it's safe to reverse https://code.launchpad.net/~cjwatson/launchpad/packageset-score-rename-support/+merge/107981 and remove name= from the property declaration.  The combination of r15343 and this patch (verified with combinediff) is simply:

-    relative_build_score = Int(name="score", allow_none=False)
+    relative_build_score = Int(allow_none=False)

== Tests ==

bin/test -vvct TestBuildPackageJobScore -t TestBuildQueueManual

== Demo and Q/A ==

None needed.
-- 
https://code.launchpad.net/~cjwatson/launchpad/packageset-score-rename-finish/+merge/110046
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/packageset-score-rename-finish into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2012-05-30 14:00:39 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2012-06-13 11:34:28 +0000
@@ -40,6 +40,7 @@
     )
 from lp.soyuz.interfaces.packageset import IPackagesetSet
 from lp.soyuz.model.buildfarmbuildjob import BuildFarmBuildJob
+from lp.soyuz.model.packageset import Packageset
 
 
 class BuildPackageJob(BuildFarmJobOldDerived, Storm):
@@ -110,7 +111,7 @@
             self.build.source_package_release.name,
             distroseries=self.build.distro_series)
         if not package_sets.is_empty():
-            score += max(ps.relative_build_score for ps in package_sets)
+            score += package_sets.max(Packageset.relative_build_score)
 
         # Calculates the build queue time component of the score.
         right_now = datetime.now(pytz.timezone('UTC'))

=== modified file 'lib/lp/soyuz/model/packageset.py'
--- lib/lp/soyuz/model/packageset.py	2012-05-30 14:00:39 +0000
+++ lib/lp/soyuz/model/packageset.py	2012-06-13 11:34:28 +0000
@@ -14,7 +14,6 @@
     Storm,
     Unicode,
     )
-from storm.store import Store
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -29,8 +28,6 @@
     IMasterStore,
     IStore,
     )
-from lp.services.database.postgresql import table_has_column
-from lp.services.database.sqlbase import cursor
 from lp.services.helpers import ensure_unicode
 from lp.soyuz.interfaces.packageset import (
     DuplicatePackagesetName,
@@ -73,52 +70,7 @@
     packagesetgroup_id = Int(name='packagesetgroup', allow_none=False)
     packagesetgroup = Reference(packagesetgroup_id, 'PackagesetGroup.id')
 
-    # Provide a manual property instead of declaring the column in Storm.
-    # This is to handle a transition period where we rename the 'score'
-    # column to 'relative_build_score'.  During the transition period, the
-    # code needs to work with both column names.
-    #
-    # The approach taken here only works because we never use 'score' in a
-    # WHERE clause or anything like that.
-    #
-    # Once the database change has taken place, this property should be
-    # deleted, and replaced with a class variable declaration as follows:
-    #
-    #   relative_build_score = Int(allow_none=False)
-
-    def _get_relative_build_score_column_name(self):
-        """Get the name of the column containing a relative build score.
-
-        Older versions of the database call it 'score'; newer ones call it
-        'relative_build_score'.
-
-        Works by interrogating PostgreSQL's own records.
-        """
-        # Chose this look-before-you-leap implementation so as to avoid
-        # invalidating the query by forcing a ProgrammingError.
-        cur = cursor()
-        if table_has_column(cur, "packageset", "score"):
-            return "score"
-        else:
-            return "relative_build_score"
-
-    @property
-    def relative_build_score(self):
-        """See `IPackageset`."""
-        store = Store.of(self)
-        store.flush()
-        column = self._get_relative_build_score_column_name()
-        query = "SELECT %s FROM packageset WHERE id=%%s" % column
-        return store.execute(query, (self.id,)).get_one()[0]
-
-    @relative_build_score.setter
-    def relative_build_score(self, score):
-        """See `IPackageset`."""
-        store = Store.of(self)
-        store.flush()
-        column = self._get_relative_build_score_column_name()
-        query = "UPDATE packageset SET %s=%%s WHERE id=%%s" % column
-        return store.execute(query, (int(score), self.id))
+    relative_build_score = Int(allow_none=False)
 
     def add(self, data):
         """See `IPackageset`."""