Code review comment for lp:~stevenk/launchpad/populate-searchables-for-pu

Revision history for this message
William Grant (wgrant) wrote :

20 + searchable_names = None
21 + searchable_versions = None
22 + if package_copy_job is not None:
23 + searchable_names = package_copy_job.package_name
24 + searchable_versions = [package_copy_job.package_version]

Could you make it clear that the non-PCJ case is handled when the PU[SBC] are created? Wouldn't it also make more sense to set searchable_names and searchable_versions to '' and [] respectively in the constructor, and then use the normal add mechanism with the PCJ data?

74 + return self.store.find(
75 + (PackageUpload.id,), PackageUpload.searchable_names == None,
76 + PackageUpload.searchable_versions == None,
77 + PackageUpload.id >= self.start_at).order_by(PackageUpload.id)

I generally prefer to the start the conditions on the line after the findspec, so it's easier to scan.

443 + def setSearchableNames(self, names):
444 + self.searchable_names = ' '.join(
445 + self._appendSearchables(self.searchable_names, names))
446 +
447 + def setSearchableVersions(self, versions):
448 + self.searchable_versions = self._appendSearchables(
449 + self.searchable_versions, versions)

These look like they actually add, not set. It would also be nice to sort the sets (probably in _appendSearchables), so that this is all deterministic.

review: Approve (code)

« Back to merge proposal