-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Jeroen, very nice branch and I learned a few new things by reviewing it. Thanks. ;-) I could not find anything serious so please bear my nit-picks with patience. And thank you for all the drive-by fixes! ;) review approve code Cheers, Henning Am 03.09.2010 08:46, schrieb Jeroen T. Vermeulen: > === modified file 'lib/lp/registry/model/pillar.py' > @@ -244,18 +230,12 @@ > stacklevel=2) > pillars = [] > # Prefill pillar.product.licenses. > - for pillar_name, other, product, project, distro, license_ids in ( > + for pillar_name, other, product, project, distro, licenses in ( > result[:limit]): You can fix this confusing indention by pulling the de-tupling into the loop body. > pillar = pillar_name.pillar > if IProduct.providedBy(pillar): > - licenses = [ > - License.items[license_id] > - for license_id in license_ids] > - product_with_licenses = ProductWithLicenses( > - pillar, tuple(sorted(licenses))) > - pillars.append(product_with_licenses) > - else: > - pillars.append(pillar) > + pillar = ProductWithLicenses(pillar, licenses) > + pillars.append(pillar) > return pillars > > def add_featured_project(self, project): > > === modified file 'lib/lp/registry/model/product.py' > @@ -201,14 +202,25 @@ > return LicenseStatus.OPEN_SOURCE > > > +class Array(NamedFunc): > + """Implements the postgres "array" function in Storm.""" > + name = 'array' > + > + > class ProductWithLicenses: > """Caches `Product.licenses`.""" > > delegates(IProduct, 'product') > > - def __init__(self, product, licenses): > + def __init__(self, product, license_ids): > + """Initialize a `ProductWithLicenses`. > + > + :param product: the `Product` to wrap. > + :param license_ids: a sequence of numeric `License` ids. > + """ > self.product = product > - self._licenses = licenses > + self._licenses = tuple([ > + License.items[id] for id in sorted(license_ids)]) You don't need the [] inside the tuple(). Go straight from iterator to tuple. (I just found that out recently myself.) > > @property > def licenses(self): > === modified file 'lib/lp/translations/interfaces/translationgroup.py' > --- lib/lp/translations/interfaces/translationgroup.py 2010-08-20 20:31:18 +0000 > +++ lib/lp/translations/interfaces/translationgroup.py 2010-09-03 06:46:46 +0000 > @@ -186,8 +186,34 @@ > def fetchTranslatorData(): > """Fetch translators and related data. > > - :return: A tuple (`Translator`, `Language`, `Person`), ordered > - by language name in English. > + Prefetches display-related properties. > + > + :return: A result set of (`Translator`, `Language`, `Person`), > + ordered by language name in English. > + """ > + > + def fetchProjectsForDisplay(): I would ask you to use "fetchProductsForDisplay" as long as it has not been renamed but I fear that others have already done the same thing in other places of the code ... This goes for all uses of "project" in this branch. > + """Fetch `Product`s using this group, for display purposes. > + > + Prefetches display-related properties. > + > + :return: A result set of `Product`, ordered by display name. > + """ > + > + def fetchProjectGroupsForDisplay(): > + """Fetch `Project`s using this group, for display purposes. > + > + Prefetches display-related properties. > + > + :return: A result set of `Project`, ordered by display name. > + """ > + > + def fetchDistrosForDisplay(): > + """Fetch `Distribution`s using this group, for display purposes. > + > + Prefetches display-related properties. > + > + :return: A result set of `Distribution`, ordered by display name. > """ > > > > === modified file 'lib/lp/translations/model/translationgroup.py' > --- lib/lp/translations/model/translationgroup.py 2010-08-20 20:31:18 +0000 > +++ lib/lp/translations/model/translationgroup.py 2010-09-03 06:46:46 +0000 > @@ -1,4 +1,4 @@ > -# Copyright 2009 Canonical Ltd. This software is licensed under the > +# Copyright 2009-2010 Canonical Ltd. This software is licensed under the > # GNU Affero General Public License version 3 (see the file LICENSE). > > # pylint: disable-msg=E0611,W0212 > @@ -6,7 +6,7 @@ > __metaclass__ = type > __all__ = [ > 'TranslationGroup', > - 'TranslationGroupSet' > + 'TranslationGroupSet', > ] > > from sqlobject import ( > @@ -16,7 +16,10 @@ > SQLRelatedJoin, > StringCol, > ) > -from storm.expr import Join > +from storm.expr import ( > + Join, > + LeftJoin, > + ) > from storm.store import Store > from zope.component import getUtility > from zope.interface import implements > @@ -24,17 +27,27 @@ > from canonical.database.constants import DEFAULT > from canonical.database.datetimecol import UtcDateTimeCol > from canonical.database.sqlbase import SQLBase > +from canonical.launchpad.database.librarian import ( > + LibraryFileAlias, > + LibraryFileContent, > + ) > from canonical.launchpad.webapp.interfaces import ( > DEFAULT_FLAVOR, > IStoreSelector, > MAIN_STORE, > + SLAVE_FLAVOR, Can you check if you cannot use ISlaveStore instead, please? > ) > from lp.app.errors import NotFoundError > from lp.registry.interfaces.person import validate_public_person > +from lp.registry.model.distribution import Distribution > from lp.registry.model.person import Person > -from lp.registry.model.product import Product > +from lp.registry.model.product import ( > + Product, > + ProductWithLicenses, > + ) > from lp.registry.model.projectgroup import ProjectGroup > from lp.registry.model.teammembership import TeamParticipation > +from lp.services.database.prejoin import prejoin > from lp.services.worlddata.model.language import Language > from lp.translations.interfaces.translationgroup import ( > ITranslationGroup, > @@ -77,10 +90,10 @@ > Translator.translationgroup == self, > Translator.languageID == Language.id, > Language.code == language_code) > - > + > translator = query.one() > if translator is None: > - raise NotFoundError, language_code > + raise NotFoundError(language_code) > > return translator > > @@ -141,14 +154,106 @@ > return 0 > > def fetchTranslatorData(self): > - """See ITranslationGroup.""" > + """See `ITranslationGroup`.""" > store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) > - translator_data = store.find( > - (Translator, Language, Person), > + # Fetch Translator, Language, and Person; but also prefetch the > + # icon information. > + using = [ > + Translator, > + Language, > + Person, > + LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Person.iconID), > + LeftJoin( > + LibraryFileContent, > + LibraryFileContent.id == LibraryFileAlias.contentID), > + ] > + tables = ( > + Translator, > + Language, > + Person, > + LibraryFileAlias, > + LibraryFileContent, > + ) > + translator_data = store.using(*using).find( > + tables, > Translator.translationgroup == self, > Language.id == Translator.languageID, > Person.id == Translator.translatorID) > - return translator_data.order_by(Language.englishname) > + > + return prejoin( > + translator_data.order_by(Language.englishname), > + slice(0, 3)) Did you know that you can apply the "order_by" to the result of "prejoin" as well? I don't know, though, if that would really look better. > + > + def fetchProjectsForDisplay(self): > + """See `ITranslationGroup`.""" > + store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR) > + using = [ > + Product, > + LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Product.iconID), > + LeftJoin( > + LibraryFileContent, > + LibraryFileContent.id == LibraryFileAlias.contentID), > + ] > + columns = ( > + Product, > + ProductWithLicenses.composeLicensesColumn(), > + LibraryFileAlias, > + LibraryFileContent, > + ) > + product_data = store.using(*using).find( > + columns, > + Product.translationgroupID == self.id, Product.active == True) > + product_data = product_data.order_by(Product.displayname) > + > + return [ > + ProductWithLicenses(product, tuple(licenses)) > + for product, licenses, icon_alias, icon_content in product_data] > + > + def fetchProjectGroupsForDisplay(self): > + """See `ITranslationGroup`.""" > + store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR) > + using = [ > + ProjectGroup, > + LeftJoin( > + LibraryFileAlias, LibraryFileAlias.id == ProjectGroup.iconID), > + LeftJoin( > + LibraryFileContent, > + LibraryFileContent.id == LibraryFileAlias.contentID), > + ] > + tables = ( > + ProjectGroup, > + LibraryFileAlias, > + LibraryFileContent, > + ) > + product_data = store.using(*using).find( > + tables, > + ProjectGroup.translationgroupID == self.id, > + ProjectGroup.active == True) > + > + return prejoin( > + product_data.order_by(ProjectGroup.displayname), slice(0, 1)) slice(0, 1) is the default for prejoin, so you could leave it out here. > + > + def fetchDistrosForDisplay(self): > + """See `ITranslationGroup`.""" > + store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR) > + using = [ > + Distribution, > + LeftJoin( > + LibraryFileAlias, LibraryFileAlias.id == Distribution.iconID), > + LeftJoin( > + LibraryFileContent, > + LibraryFileContent.id == LibraryFileAlias.contentID), > + ] > + tables = ( > + Distribution, > + LibraryFileAlias, > + LibraryFileContent, > + ) > + product_data = store.using(*using).find( > + tables, Distribution.translationgroupID == self.id) > + > + return prejoin( > + product_data.order_by(Distribution.displayname), slice(0, 1)) > > > class TranslationGroupSet: > @@ -174,7 +279,7 @@ > try: > return TranslationGroup.byName(name) > except SQLObjectNotFound: > - raise NotFoundError, name > + raise NotFoundError(name) > > def new(self, name, title, summary, translation_guide_url, owner): > """See ITranslationGroupSet.""" > @@ -193,7 +298,7 @@ > Join(Translator, > Translator.translationgroupID == TranslationGroup.id), > Join(TeamParticipation, > - TeamParticipation.teamID == Translator.translatorID) > + TeamParticipation.teamID == Translator.translatorID), > ] > result = store.using(*origin).find( > TranslationGroup, TeamParticipation.person == person) > @@ -203,4 +308,3 @@ > def getGroupsCount(self): > """See ITranslationGroupSet.""" > return TranslationGroup.select().count() > - > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkyAzUwACgkQBT3oW1L17ijQzACg4i1Md61gBrPM6ddIkY/jIwu+ BXEAniaqu+AOAacWo/CaTcGskK+b9+0d =Q6P5 -----END PGP SIGNATURE-----