Thanks for the review Edwin. I have made your requested changes, and I identified some additional changes needed by the view in my next branch. I think you should review this. On Wed, 2010-01-20 at 18:06 +0000, Edwin Grubbs wrote: > Review: Approve code > Hi Curtis, > > This branch makes a nice coherent chunk. My suggestions are fairly minor. > > merge-conditional > >=== modified file 'lib/lp/registry/doc/distroseries.txt' > >--- lib/lp/registry/doc/distroseries.txt 2009-12-10 20:28:03 +0000 > >+++ lib/lp/registry/doc/distroseries.txt 2010-01-20 16:13:26 +0000 > >@@ -500,7 +500,35 @@ > > netapplet are translatable in Hoary. > > > > > >-== DistroSeries can build meta objects for packages == > >+Packages that need linking and packagings that need upstream information > >+----------------------------------------------------------------------- > >+ > >+A distroseries a getPriorizedUnlinkedSourcePackages() method that returns > > The beginning of this sentence is confusing. The distroseries getPriorizedUnlinkedSourcePackages() > >+a prioritized list of `ISourcePackage` that need a packaging link to an > > Since `ISourcePackage` is not pluralized, it feels odd when I get > to the verb "need" which implies that the noun should be plural. a prioritized list of `ISourcePackage` objects > >+`IProductSeries` to provide the the upstream information to share bugs, > >+translations, and code. > >+ > >+ >>> for source_package in hoary.getPriorizedUnlinkedSourcePackages(): > >+ ... print source_package.name > >+ pmount > >+ alsa-utils > >+ cnews > >+ libstdc++ > >+ linux-source-2.6.15 This test changed because the view will need a dict, see the diff. > >+A distroseries a getPriorizedlPackagings() method that returns a prioritized > > Beginning of this sentence also. The distroseries getPriorizedlPackagings() method > >=== modified file 'lib/lp/registry/model/distroseries.py' > >--- lib/lp/registry/model/distroseries.py 2009-12-22 17:41:46 +0000 > >+++ lib/lp/registry/model/distroseries.py 2010-01-20 16:13:26 +0000 ... > >+ def getPriorizedlPackagings(self): > >+ """See `IDistroSeries`. > >+ > >+ The prioritization is a heuristic rule using the branch, bug hotness, > > s/using the/using the/ Fixed. > >+ translatable messages, and the source package release's component. > >+ """ > >+ # Avoid circular import failures. > >+ # We join to SourcePackageName, ProductSeries, and Product to cache > >+ # the objects that are implcitly needed to work with a > >+ # Packaging object. > >+ from lp.registry.model.product import Product > >+ from lp.registry.model.productseries import ProductSeries > > It seems like it would be clearer to move these imports directly > below the comment line about "Avoid circular import failures." Fixed. I tried to remove this since Adi's branch addressed part of the problem, but I abandoned my effort when the branch grew 250 lines and no clue as to what else needed to change. > >+ find_spec = ( > >+ Packaging, SourcePackageName, ProductSeries, Product, > >+ SQL(""" > >+ CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END + > >+ CASE WHEN Product.bugtracker IS NULL > >+ THEN coalesce(heat, 10) ELSE 0 END + > >+ CASE WHEN ProductSeries.translations_autoimport_mode = 1 > >+ THEN coalesce(po_messages, 10) ELSE 0 END + > >+ CASE WHEN ProductSeries.branch IS NULL THEN 500 > >+ ELSE 0 END AS score""")) > >+ joins, conditions = self._current_sourcepackage_joins_and_conditions > >+ origin = SQL(joins + """ > >+ JOIN ProductSeries > >+ ON Packaging.productseries = ProductSeries.id > >+ JOIN Product > >+ ON ProductSeries.product = Product.id > >+ """) > >+ condition = SQL(conditions + "AND packaging.id IS NOT NULL") > >+ results = IStore(self).using(origin).find(find_spec, condition) > >+ results = results.order_by('score DESC') > >+ return [packaging > >+ for (packaging, spn, series, product, score) in results] > >+ > >+ @property > >+ def _current_sourcepackage_joins_and_conditions(self): > >+ """The SQL joins and conditions to prioritise source packages.""" > >+ heat_score = (""" > >+ LEFT JOIN ( > >+ SELECT > >+ BugTask.sourcepackagename, > >+ sum(Bug.hotness) AS heat > > It's not obvious elsewhere what the difference is between heat and > hotness. Can you rename it either hotness_sum, heat_sum, or total_heat? I choose total_heat because the column is/has been renamed to heat in db-devel. I need to do an additional reconciliation to land this. > >+ FROM BugTask > >+ JOIN Bug > >+ ON bugtask.bug = Bug.id > >+ WHERE > >+ BugTask.sourcepackagename is not NULL > >+ AND BugTask.distribution = %s > >+ AND BugTask.status in %s > >+ GROUP BY BugTask.sourcepackagename > >+ ) bugs > >+ ON SourcePackageName.id = bugs.sourcepackagename > >+ """ % sqlvalues(self.distribution, UNRESOLVED_BUGTASK_STATUSES)) > >+ message_score = (""" > >+ LEFT JOIN ( > >+ SELECT > >+ POTemplate.sourcepackagename, > >+ POTemplate.distroseries, > >+ SUM(POTemplate.messagecount) / 2 AS po_messages > >+ FROM POTemplate > >+ WHERE > >+ POTemplate.sourcepackagename is not NULL > >+ AND POTemplate.distroseries = %s > >+ GROUP BY > >+ POTemplate.sourcepackagename, > >+ POTemplate.distroseries > >+ ) messages > >+ ON SourcePackageName.id = messages.sourcepackagename > >+ AND DistroSeries.id = messages.distroseries > >+ """ % sqlvalues(self)) > >+ joins = (""" > >+ SourcePackageName > >+ JOIN SourcePackageRelease spr > >+ ON SourcePackageName.id = spr.sourcepackagename > >+ JOIN SourcePackagePublishingHistory spph > >+ ON spr.id = spph.sourcepackagerelease > >+ JOIN archive > >+ ON spph.archive = Archive.id > >+ JOIN DistroSeries > >+ ON spph.distroseries = DistroSeries.id > >+ LEFT JOIN Packaging > >+ ON SourcePackageName.id = Packaging.sourcepackagename > >+ AND Packaging.distroseries = DistroSeries.id > >+ """ + heat_score + message_score) > > I'm wondering if this would be easier to read if they were > defined as a single string variable with keyword substitution using > sqlvalues(distro=self.distribution, distroseries=self, statuses=...) I broke the large query up because I think it is more readable. ... > >=== modified file 'lib/lp/registry/tests/test_distroseries.py' > >--- lib/lp/registry/tests/test_distroseries.py 2010-01-07 06:45:03 +0000 > >+++ lib/lp/registry/tests/test_distroseries.py 2010-01-20 16:13:26 +0000 ... > >+class TestDistroSeriesPackaging(TestCaseWithFactory): ... > >+ def test_getPriorizedlPackagings_bug_tracker(self): > >+ # Verify the ordering of packagings without and without a bug tracker. > > > Double withouts. Fixed. > >+ self.linkPackage('hot') > >+ self.makeSeriesPackage('cold') > >+ product_series = self.linkPackage('cold') > >+ product_series.product.bugtraker = self.factory.makeBugTracker() > >+ packagings = self.series.getPriorizedlPackagings() > >+ names = [packaging.sourcepackagename.name for packaging in packagings] > >+ expected = [u'hot', u'linked', u'cold'] > >+ self.assertEqual(expected, names) > >+ > >+ def test_getPriorizedlPackagings_branch(self): > >+ # Verify the ordering of packagings without and without a branch. > > > Again. Fixed. ... This incremental diff includes your suggestions and a change in getPriorizedUnlinkedSourcePackages() to return a dict of package, total_bugs, and total_messages. The view needs these last two pieces information and it is expensive to retrieve. {{{ === modified file 'lib/lp/registry/doc/distroseries.txt' --- lib/lp/registry/doc/distroseries.txt 2010-01-20 23:29:21 +0000 +++ lib/lp/registry/doc/distroseries.txt 2010-01-21 00:33:26 +0000 @@ -503,21 +503,23 @@ Packages that need linking and packagings that need upstream information ----------------------------------------------------------------------- -A distroseries a getPriorizedUnlinkedSourcePackages() method that returns -a prioritized list of `ISourcePackage` that need a packaging link to an -`IProductSeries` to provide the the upstream information to share bugs, -translations, and code. - - >>> for package_summary in hoary.getPriorizedUnlinkedSourcePackages(): - ... print package_summary['package'].name - pmount - alsa-utils - cnews - libstdc++ - linux-source-2.6.15 - - -A distroseries a getPriorizedlPackagings() method that returns a prioritized +The distroseries getPriorizedUnlinkedSourcePackages() method that returns +a prioritized list of `ISourcePackage` objects that need a packaging link to +an `IProductSeries` to provide the the upstream information to share bugs, +translations, and code. Each item in the list is a dict with the 'package', +total_bugs, and total_messages (translatable messages). + + >>> for summary in hoary.getPriorizedUnlinkedSourcePackages(): + ... print summary['package'].name + ... print '%(total_bugs)s %(total_messages)s' % summary + pmount 0 64 + alsa-utils 0 0 + cnews 0 0 + libstdc++ 0 0 + linux-source-2.6.15 1 0 + + +The distroseries getPriorizedlPackagings() method that returns a prioritized list of `IPackaging` that need more information about the upstream project to share bugs, translations, and code. === modified file 'lib/lp/registry/interfaces/distroseries.py' --- lib/lp/registry/interfaces/distroseries.py 2010-01-14 22:40:51 +0000 +++ lib/lp/registry/interfaces/distroseries.py 2010-01-20 22:49:33 +0000 @@ -489,7 +489,11 @@ """ def getPriorizedUnlinkedSourcePackages(): - """Return a list of source packages that need packaging links.""" + """Return a list of package summaries that need packaging links. + + A summary is a dict of package (`ISourcePackage`), total_bugs, + and total_messages (translatable messages). + """ def getPriorizedlPackagings(): """Return a list of packagings that need more upstream information.""" === modified file 'lib/lp/registry/model/distroseries.py' --- lib/lp/registry/model/distroseries.py 2010-01-20 23:29:21 +0000 +++ lib/lp/registry/model/distroseries.py 2010-01-21 14:50:57 +0000 @@ -306,10 +306,10 @@ @property def packagings(self): """See `IDistroSeries`.""" - # Avoid circular import failures. # We join to SourcePackageName, ProductSeries, and Product to cache # the objects that are implicitly needed to work with a # Packaging object. + # Avoid circular import failures. from lp.registry.model.product import Product from lp.registry.model.productseries import ProductSeries find_spec = (Packaging, SourcePackageName, ProductSeries, Product) @@ -340,7 +340,7 @@ find_spec = ( SourcePackageName, SQL(""" - coalesce(heat, 0) + coalesce(po_messages, 0) + + coalesce(total_heat, 0) + coalesce(po_messages, 0) + - CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END AS score""")) + CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END AS score"""), + SQL("coalesce(total_bugs, 0) AS total_bugs"), + SQL("coalesce(total_messages, 0) AS total_messages")) joins, conditions = self._current_sourcepackage_joins_and_conditions origin = SQL(joins) condition = SQL(conditions + "AND packaging.id IS NULL") results = IStore(self).using(origin).find(find_spec, condition) results = results.order_by('score DESC') - return [SourcePackage(sourcepackagename=spn, distroseries=self) - for (spn, score) in results] + return [{ + 'package': SourcePackage( + sourcepackagename=spn, distroseries=self), + 'total_bugs': total_bugs, + 'total_messages': total_messages} + for (spn, score, total_bugs, total_messages) in results] def getPriorizedlPackagings(self): """See `IDistroSeries`. - The prioritization is a heuristic rule using the branch, bug hotness, + The prioritization is a heuristic rule using the branch, bug hotness, translatable messages, and the source package release's component. """ - # Avoid circular import failures. # We join to SourcePackageName, ProductSeries, and Product to cache # the objects that are implcitly needed to work with a # Packaging object. + # Avoid circular import failures. from lp.registry.model.product import Product from lp.registry.model.productseries import ProductSeries find_spec = ( @@ -373,7 +373,7 @@ SQL(""" CASE WHEN spr.component = 1 THEN 1000 ELSE 0 END + CASE WHEN Product.bugtracker IS NULL - THEN coalesce(heat, 10) ELSE 0 END + + THEN coalesce(total_heat, 10) ELSE 0 END + CASE WHEN ProductSeries.translations_autoimport_mode = 1 THEN coalesce(po_messages, 10) ELSE 0 END + CASE WHEN ProductSeries.branch IS NULL THEN 500 @@ -398,19 +398,21 @@ LEFT JOIN ( SELECT BugTask.sourcepackagename, - sum(Bug.hotness) AS heat, + sum(Bug.hotness) AS total_heat, + count(Bug.id) AS total_bugs FROM BugTask JOIN Bug ON bugtask.bug = Bug.id WHERE BugTask.sourcepackagename is not NULL - AND BugTask.distribution = %s - AND BugTask.status in %s + AND BugTask.distribution = %(distribution)s + AND BugTask.status in %(statuses)s GROUP BY BugTask.sourcepackagename ) bugs ON SourcePackageName.id = bugs.sourcepackagename - """ % sqlvalues(self.distribution, UNRESOLVED_BUGTASK_STATUSES)) + """ % sqlvalues( + distribution=self.distribution, + statuses=UNRESOLVED_BUGTASK_STATUSES)) message_score = (""" LEFT JOIN ( SELECT POTemplate.sourcepackagename, POTemplate.distroseries, - SUM(POTemplate.messagecount) / 2 AS po_messages + SUM(POTemplate.messagecount) / 2 AS po_messages, + SUM(POTemplate.messagecount) AS total_messages FROM POTemplate WHERE POTemplate.sourcepackagename is not NULL - AND POTemplate.distroseries = %s + AND POTemplate.distroseries = %(distroseries)s GROUP BY POTemplate.sourcepackagename, POTemplate.distroseries ) messages ON SourcePackageName.id = messages.sourcepackagename AND DistroSeries.id = messages.distroseries - """ % sqlvalues(self)) + """ % sqlvalues(distroseries=self)) joins = (""" SourcePackageName JOIN SourcePackageRelease spr @@ -444,11 +446,13 @@ AND Packaging.distroseries = DistroSeries.id """ + heat_score + message_score) conditions = (""" - DistroSeries.id = %s - AND spph.status IN %s - AND archive.purpose = %s + DistroSeries.id = %(distroseries)s + AND spph.status IN %(active_status)s + AND archive.purpose = %(primary)s """ % sqlvalues( - self, active_publishing_status, ArchivePurpose.PRIMARY)) + distroseries=self, + active_status=active_publishing_status, + primary=ArchivePurpose.PRIMARY)) return (joins, conditions) @property === modified file 'lib/lp/registry/tests/test_distroseries.py' --- lib/lp/registry/tests/test_distroseries.py 2010-01-20 22:49:33 +0000 +++ lib/lp/registry/tests/test_distroseries.py 2010-01-21 14:30:33 +0000 @@ -234,8 +234,8 @@ def test_getPriorizedUnlinkedSourcePackages(self): # Verify the ordering of source packages that need linking. - source_packages = self.series.getPriorizedUnlinkedSourcePackages() - names = [package.name for package in source_packages] + package_summaries = self.series.getPriorizedUnlinkedSourcePackages() + names = [summary['package'].name for summary in package_summaries] expected = [ u'main', u'hot-translatable', u'hot', u'translatable', u'normal'] self.assertEqual(expected, names) def test_getPriorizedlPackagings_bug_tracker(self): - # Verify the ordering of packagings without and without a bug tracker. + # Verify the ordering of packagings with and without a bug tracker. self.linkPackage('hot') self.makeSeriesPackage('cold') product_series = self.linkPackage('cold') @@ -262,7 +262,7 @@ self.assertEqual(expected, names) def test_getPriorizedlPackagings_branch(self): - # Verify the ordering of packagings without and without a branch. + # Verify the ordering of packagings with and without a branch. self.linkPackage('translatable') self.makeSeriesPackage('withbranch') product_series = self.linkPackage('withbranch') }}} -- __Curtis C. Hovey_________ http://launchpad.net/