launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02322
[Merge] lp:~julian-edwards/launchpad/count-cull into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/count-cull into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#701947 Soyuz code uses .count() too much when all it needs is a bool
https://bugs.launchpad.net/bugs/701947
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/count-cull/+merge/46158
= Summary =
Cull a load of unnecessary .count()s in favour of less-expensive SQL.
== Proposed fix ==
See bug 701947
== Implementation details ==
See bug 701947
I had trouble converting one of them as you can see from the comment, I filed
bug 702425 about it.
== Demo and Q/A ==
n/a
--
https://code.launchpad.net/~julian-edwards/launchpad/count-cull/+merge/46158
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/count-cull into lp:launchpad.
=== modified file 'lib/lp/soyuz/adapters/archivedependencies.py'
--- lib/lp/soyuz/adapters/archivedependencies.py 2010-08-24 15:29:01 +0000
+++ lib/lp/soyuz/adapters/archivedependencies.py 2011-01-13 17:41:26 +0000
@@ -122,12 +122,10 @@
name=sourcepackagename,
distroseries=distroseries, exact_match=True)
- # XXX cprov 20080923 bug=246200: This count should be replaced
- # by bool() (__non_zero__) when storm implementation gets fixed.
- if ancestries.count() > 0:
+ try:
return ancestries[0].component.name
-
- return 'universe'
+ except IndexError:
+ return 'universe'
def expand_dependencies(archive, distro_series, pocket, component,
=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py 2010-12-20 05:46:40 +0000
+++ lib/lp/soyuz/browser/archive.py 2011-01-13 17:41:26 +0000
@@ -392,9 +392,9 @@
if the_item is not None:
result_set = getUtility(IArchivePermissionSet).checkAuthenticated(
user, self.context, permission_type, the_item)
- if result_set.count() > 0:
+ try:
return result_set[0]
- else:
+ except IndexError:
return None
else:
return None
@@ -570,9 +570,7 @@
the view to determine whether to display "This PPA does not yet
have any published sources" or "No sources matching 'blah'."
"""
- # XXX cprov 20080708 bug=246200: use bool() when it gets fixed
- # in storm.
- return self.context.getPublishedSources().count() > 0
+ return bool(self.context.getPublishedSources())
@cachedproperty
def repository_usage(self):
@@ -851,8 +849,8 @@
This is after any filtering or overriding of the sources() method.
"""
- # XXX cprov 20080708 bug=246200: use bool() when it gets fixed
- # in storm.
+ # Trying to use bool(self.filtered_sources) here resulted in bug
+ # 702425 :(
return self.filtered_sources.count() > 0
@@ -1170,9 +1168,7 @@
to ensure that it only returns true if there are sources
that can be deleted in this archive.
"""
- # XXX cprov 20080708 bug=246200: use bool() when it gets fixed
- # in storm.
- return self.context.getSourcesForDeletion().count() > 0
+ return bool(self.context.getSourcesForDeletion())
def validate_delete(self, action, data):
"""Validate deletion parameters.
@@ -1626,9 +1622,7 @@
@cachedproperty
def has_dependencies(self):
"""Whether or not the PPA has recorded dependencies."""
- # XXX cprov 20080708 bug=246200: use bool() when it gets fixed
- # in storm.
- return self.context.dependencies.count() > 0
+ return bool(self.context.dependencies)
@property
def messages(self):
@@ -1945,7 +1939,7 @@
if data.get('private') != self.context.private:
# The privacy is being switched.
- if self.context.getPublishedSources().count() > 0:
+ if bool(self.context.getPublishedSources()):
self.setFieldError(
'private',
'This archive already has published sources. It is '
=== modified file 'lib/lp/soyuz/browser/build.py'
--- lib/lp/soyuz/browser/build.py 2010-12-16 12:48:14 +0000
+++ lib/lp/soyuz/browser/build.py 2011-01-13 17:41:26 +0000
@@ -182,7 +182,7 @@
# are always published.
imported_binaries = (
self.package_upload is None and
- self.context.binarypackages.count() > 0)
+ bool(self.context.binarypackages))
# Binaries uploaded from the buildds are published when the
# corresponding `PackageUpload` status is DONE.
uploaded_binaries = (
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2011-01-03 22:15:46 +0000
+++ lib/lp/soyuz/model/archive.py 2011-01-13 17:41:26 +0000
@@ -1210,7 +1210,7 @@
def _authenticate(self, user, component, permission):
"""Private helper method to check permissions."""
permissions = self.getPermissions(user, component, permission)
- return permissions.count() > 0
+ return bool(permissions)
def newPackageUploader(self, person, source_package_name):
"""See `IArchive`."""
=== modified file 'lib/lp/soyuz/model/archivepermission.py'
--- lib/lp/soyuz/model/archivepermission.py 2010-11-05 09:16:14 +0000
+++ lib/lp/soyuz/model/archivepermission.py 2011-01-13 17:41:26 +0000
@@ -295,34 +295,37 @@
sourcepackagename = self._nameToSourcePackageName(sourcepackagename)
existing = self.checkAuthenticated(
person, archive, ArchivePermissionType.UPLOAD, sourcepackagename)
- if existing.count() != 0:
+ try:
return existing[0]
- return ArchivePermission(
- archive=archive, person=person,
- sourcepackagename=sourcepackagename,
- permission=ArchivePermissionType.UPLOAD)
+ except IndexError:
+ return ArchivePermission(
+ archive=archive, person=person,
+ sourcepackagename=sourcepackagename,
+ permission=ArchivePermissionType.UPLOAD)
def newComponentUploader(self, archive, person, component):
"""See `IArchivePermissionSet`."""
component = self._nameToComponent(component)
existing = self.checkAuthenticated(
person, archive, ArchivePermissionType.UPLOAD, component)
- if existing.count() != 0:
+ try:
return existing[0]
- return ArchivePermission(
- archive=archive, person=person, component=component,
- permission=ArchivePermissionType.UPLOAD)
+ except IndexError:
+ return ArchivePermission(
+ archive=archive, person=person, component=component,
+ permission=ArchivePermissionType.UPLOAD)
def newQueueAdmin(self, archive, person, component):
"""See `IArchivePermissionSet`."""
component = self._nameToComponent(component)
existing = self.checkAuthenticated(
person, archive, ArchivePermissionType.QUEUE_ADMIN, component)
- if existing.count() != 0:
+ try:
return existing[0]
- return ArchivePermission(
- archive=archive, person=person, component=component,
- permission=ArchivePermissionType.QUEUE_ADMIN)
+ except IndexError:
+ return ArchivePermission(
+ archive=archive, person=person, component=component,
+ permission=ArchivePermissionType.QUEUE_ADMIN)
def deletePackageUploader(self, archive, person, sourcepackagename):
"""See `IArchivePermissionSet`."""
=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py 2010-11-11 11:55:53 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py 2011-01-13 17:41:26 +0000
@@ -542,7 +542,7 @@
'SourcePackageRelease'])
estimated_duration = None
- if completed_builds.count() > 0:
+ if bool(completed_builds):
# Historic build data exists, use the most recent value -
# assuming it has valid data.
most_recent_build = completed_builds[0]
@@ -569,7 +569,7 @@
return estimated_duration
def verifySuccessfulUpload(self):
- return self.binarypackages.count() > 0
+ return bool(self.binarypackages)
def notify(self, extra_info=None):
"""See `IPackageBuild`.
=== modified file 'lib/lp/soyuz/model/distroseriessourcepackagerelease.py'
--- lib/lp/soyuz/model/distroseriessourcepackagerelease.py 2010-11-06 12:08:24 +0000
+++ lib/lp/soyuz/model/distroseriessourcepackagerelease.py 2011-01-13 17:41:26 +0000
@@ -198,9 +198,10 @@
this release is or was published.
"""
pub_hist = self.publishing_history
- if pub_hist.count() == 0:
+ try:
+ return pub_hist[0]
+ except IndexError:
return None
- return pub_hist[0]
@property
def current_published(self):
=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py 2010-12-16 20:01:09 +0000
+++ lib/lp/soyuz/model/publishing.py 2011-01-13 17:41:26 +0000
@@ -1292,7 +1292,7 @@
status=active_publishing_status, pocket=pocket,
distroarchseries=distroarchseries)
- if binary_in_destination.count() == 0:
+ if not bool(binary_in_destination):
pub = BinaryPackagePublishingHistory(
archive=archive,
binarypackagerelease=binarypackagerelease,
@@ -1878,7 +1878,7 @@
name=package_name, exact_match=True, pocket=pocket,
status=status, distroseries=distroseries)
- if ancestries.count() > 0:
+ try:
return ancestries[0]
-
- return None
+ except IndexError:
+ return None
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2010-11-06 06:09:45 +0000
+++ lib/lp/soyuz/model/queue.py 2011-01-13 17:41:26 +0000
@@ -360,7 +360,7 @@
def _giveKarma(self):
"""Assign karma as appropriate for an accepted upload."""
# Give some karma to the uploader for source uploads only.
- if self.sources.count() == 0:
+ if not bool(self.sources):
return
changed_by = self.sources[0].sourcepackagerelease.creator
@@ -473,8 +473,8 @@
def _isSingleSourceUpload(self):
"""Return True if this upload contains only a single source."""
return ((self.sources.count() == 1) and
- (self.builds.count() == 0) and
- (self.customfiles.count() == 0))
+ (not bool(self.builds)) and
+ (not bool(self.customfiles)))
# XXX cprov 2006-03-14: Following properties should be redesigned to
# reduce the duplicated code.
@@ -591,9 +591,9 @@
and I am hence introducing this non-cached variant for
usage inside the content class.
"""
- if self.sources is not None and self.sources.count() > 0:
+ if self.sources is not None and bool(self.sources):
return self.sources[0].sourcepackagerelease
- elif self.builds is not None and self.builds.count() > 0:
+ elif self.builds is not None and bool(self.builds):
return self.builds[0].build.source_package_release
else:
return None
@@ -1387,7 +1387,7 @@
section=new_section,
priority=new_priority)
- return self.builds.count() > 0
+ return bool(self.builds)
class PackageUploadBuild(SQLBase):
@@ -1532,9 +1532,10 @@
name=self.sourcepackagerelease.name,
distroseries=distroseries, pocket=pocket,
exact_match=True)
- if ancestries.count() == 0:
+ try:
+ ancestry = ancestries[0]
+ except IndexError:
continue
- ancestry = ancestries[0]
break
return ancestry
=== modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py'
--- lib/lp/soyuz/scripts/initialise_distroseries.py 2010-12-21 00:26:41 +0000
+++ lib/lp/soyuz/scripts/initialise_distroseries.py 2011-01-13 17:41:26 +0000
@@ -89,7 +89,7 @@
pending_builds = self.parent.getBuildRecords(
BuildStatus.NEEDSBUILD, pocket=PackagePublishingPocket.RELEASE)
- if pending_builds.count():
+ if pending_builds.any():
raise InitialisationError("Parent series has pending builds.")
def _checkQueue(self):
@@ -114,16 +114,16 @@
error = (
"Can not copy distroarchseries from parent, there are "
"already distroarchseries(s) initialised for this series.")
- if sources.count():
+ if bool(sources):
raise InitialisationError(error)
binaries = self.distroseries.getAllPublishedBinaries()
- if binaries.count():
- raise InitialisationError(error)
- if self.distroseries.architectures.count():
- raise InitialisationError(error)
- if self.distroseries.components.count():
- raise InitialisationError(error)
- if self.distroseries.sections.count():
+ if bool(binaries):
+ raise InitialisationError(error)
+ if bool(self.distroseries.architectures):
+ raise InitialisationError(error)
+ if bool(self.distroseries.components):
+ raise InitialisationError(error)
+ if bool(self.distroseries.sections):
raise InitialisationError(error)
def initialise(self):
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2010-08-27 11:19:54 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2011-01-13 17:41:26 +0000
@@ -264,7 +264,7 @@
# If there are no conflicts with the same version, we can skip the
# rest of the checks, but we still want to check conflicting files
- if (destination_archive_conflicts.count() == 0 and
+ if (not bool(destination_archive_conflicts) and
len(inventory_conflicts) == 0):
self._checkConflictingFiles(source)
return
@@ -555,7 +555,7 @@
version=source.sourcepackagerelease.version,
status=active_publishing_status,
distroseries=series, pocket=pocket)
- if source_in_destination.count() == 0:
+ if not bool(source_in_destination):
source_copy = source.copyTo(series, pocket, archive)
close_bugs_for_sourcepublication(source_copy)
copies.append(source_copy)
=== modified file 'lib/lp/soyuz/scripts/ppa_add_missing_builds.py'
--- lib/lp/soyuz/scripts/ppa_add_missing_builds.py 2010-10-17 09:04:48 +0000
+++ lib/lp/soyuz/scripts/ppa_add_missing_builds.py 2011-01-13 17:41:26 +0000
@@ -53,7 +53,7 @@
sources = ppa.getPublishedSources(
distroseries=distroseries,
status=PackagePublishingStatus.PUBLISHED)
- if not sources.count():
+ if not bool(sources):
self.logger.info("No sources published, nothing to do.")
return
=== modified file 'lib/lp/soyuz/scripts/publishdistro.py'
--- lib/lp/soyuz/scripts/publishdistro.py 2010-08-23 16:51:11 +0000
+++ lib/lp/soyuz/scripts/publishdistro.py 2011-01-13 17:41:26 +0000
@@ -181,10 +181,7 @@
elif options.copy_archive:
archives = getUtility(IArchiveSet).getArchivesForDistribution(
distribution, purposes=[ArchivePurpose.COPY])
- # XXX 2010-02-24 Julian bug=246200
- # Fix this to use bool when Storm fixes __nonzero__ on sqlobj
- # result sets.
- if archives.count() == 0:
+ if not bool(archives):
raise LaunchpadScriptFailure("Could not find any COPY archives")
else:
archives = [distribution.main_archive]
