Launchpad itself

Code review comment for lp://staging/~adeuring/launchpad/bug-594247

bug-594247
Merge into devel

Revision history for this message

Gavin Panella (allenap) wrote on 2010-11-12:

Cool :) Lots of comments, but none of them are very important.

[1]

+ Or(BugTask.sourcepackagenameID ==
+ StructuralSubscription.sourcepackagenameID,
+ StructuralSubscription.sourcepackagename == None)))

Indentation is a little confusing there.

[2]

I had trouble reading this, so I refactored it to use more descriptive
names. If you like what I've done then I think it would be a good
change to make:

    ssub_match_product = (
        BugTask.productID ==
        StructuralSubscription.productID)
    ssub_match_productseries = (
        BugTask.productseriesID ==
        StructuralSubscription.productseriesID)
    ssub_match_project = And(
        Product.projectID ==
        StructuralSubscription.projectID,
        BugTask.product == Product.id)
    ssub_match_distribution = (
        BugTask.distributionID ==
        StructuralSubscription.distributionID)
    ssub_match_sourcepackagename = (
        BugTask.sourcepackagenameID ==
        StructuralSubscription.sourcepackagenameID)
    ssub_match_null_sourcepackagename = (
        StructuralSubscription.sourcepackagename == None)
    ssub_match_distribution_with_optional_package = And(
        ssub_match_distribution, Or(
            ssub_match_sourcepackagename,
            ssub_match_null_sourcepackagename))
    ssub_match_distribution_series = (
        BugTask.distroseriesID ==
        StructuralSubscription.distroseriesID)
    ssub_match_milestone = (
        BugTask.milestoneID ==
        StructuralSubscription.milestoneID)

    join_clause = Or(
        ssub_match_product,
        ssub_match_productseries,
        ssub_match_project,
        ssub_match_distribution_with_optional_package,
        ssub_match_distribution_series,
        ssub_match_milestone)

[3]

+ query, clauseTables, orderby_arg, decorator, join_tables,

Just because everything else seems to not be camel-case any more,
consider doing s/clauseTables/clause_tables/g.

[4]

+ origin = [BugTask]
+ already_joined = set(origin)
+ for table, join in join_tables:
+ origin.append(join)
+ already_joined.add(table)
+ for table, join in prejoin_tables:
+ if table not in already_joined:
+ origin.append(join)
+ already_joined.add(table)
+ for table in clauseTables:
+ if table not in already_joined:
+ origin.append(table)
+ return origin

Does the order matter? If not, this could be:

table_joins = chain(join_tables, prejoin_tables)
return set(table for (table, join) in table_joins).union(clauseTables)

[5]

+ (query, clauseTables, orderby, bugtask_decorator, join_tables,
+ has_duplicate_results) = self.buildQuery(params)

I /think/ we're meant to use [] for unwrapping.

[6]

+ resultset = store.using(*outer_origin).find(
+ resultrow, In(BugTask.id, subquery))

Perhaps it's worth documenting that resultrow must be BugTask, a
column of BugTask, or a sequence containing either of the above?
Assuming that's correct.

[7]

+ decorator=lambda row: bugtask_decorator(row[0])

s/=/ = /

Ah, this means that the rule in [6] should be modified to be:

resultrow must be BugTask, a column of BugTask, or a sequence whose
first element is either of the above.

It might be worth adding an assert to that.

[8]

+ undecorated = kw.get('undecorated', False)
...
+ if prejoins:
+ decorator=lambda row: bugtask_decorator(row[0])
+ else:
                 decorator = bugtask_decorator
...
+ if undecorated:
+ return resultset
             return DecoratedResultSet(resultset, result_decorator=decorator)
...
+ if undecorated:
+ return result
         return DecoratedResultSet(result, result_decorator=decorator)

I think the undecorated argument is a slightly ugly wart. Instead,
consider always returning the DecoratedResultSet, and change
searchBugIds() to:

return self._search(BugTask.bugID, [], params).result_set

[9]

+ # We need to process subscriptions, so pull all the
+ # subscribes into the cache, then update recipients
+ # with the subscriptions.

s/subscribes/subscribers/

[10]

+class ProjectGroupAndDistributionTests:
+ """Tests which are useful for project groups and distributions."""

These are about verifying distinct results. Consider renaming and
changing the docstring to reflect that.

[11]

+ BugTaskSearchParams instancs, while BugTaskSet.searchBugIds()

s/instancs/instances/

Cool :) Lots of comments, but none of them are very important.

[1]

+                    Or(BugTask.sourcepackagenameID ==
+                       StructuralSubscription.sourcepackagenameID,
+                    StructuralSubscription.sourcepackagename == None)))

Indentation is a little confusing there.

[2]

+            join_clause = (
+                BugTask.productID == StructuralSubscription.productID)
+            join_clause = Or(
+                join_clause,
+                (BugTask.productseriesID ==
+                 StructuralSubscription.productseriesID))
+            # Circular.
+            from lp.registry.model.product import Product
+            join_clause = Or(
+                join_clause,
+                And(Product.projectID == StructuralSubscription.projectID,
+                    BugTask.product == Product.id))
+            join_clause = Or(
+                join_clause,
+                And((BugTask.distributionID ==
+                     StructuralSubscription.distributionID),
+                    Or(BugTask.sourcepackagenameID ==
+                       StructuralSubscription.sourcepackagenameID,
+                    StructuralSubscription.sourcepackagename == None)))
+            join_clause = Or(
+                join_clause,
+                (BugTask.distroseriesID ==
+                 StructuralSubscription.distroseriesID))
+            join_clause = Or(
+                join_clause,
+                BugTask.milestoneID == StructuralSubscription.milestoneID)

I had trouble reading this, so I refactored it to use more descriptive
names. If you like what I've done then I think it would be a good
change to make:

[3]

+            query, clauseTables, orderby_arg, decorator, join_tables,

Just because everything else seems to not be camel-case any more,
consider doing s/clauseTables/clause_tables/g.

[4]

+        origin = [BugTask]
+        already_joined = set(origin)
+        for table, join in join_tables:
+            origin.append(join)
+            already_joined.add(table)
+        for table, join in prejoin_tables:
+            if table not in already_joined:
+                origin.append(join)
+                already_joined.add(table)
+        for table in clauseTables:
+            if table not in already_joined:
+                origin.append(table)
+        return origin

Does the order matter? If not, this could be:

table_joins = chain(join_tables, prejoin_tables)
    return set(table for (table, join) in table_joins).union(clauseTables)

[5]

+        (query, clauseTables, orderby, bugtask_decorator, join_tables,
+        has_duplicate_results) = self.buildQuery(params)

I /think/ we're meant to use [] for unwrapping.

[6]

+                resultset = store.using(*outer_origin).find(
+                    resultrow, In(BugTask.id, subquery))

Perhaps it's worth documenting that resultrow must be BugTask, a
column of BugTask, or a sequence containing either of the above?
Assuming that's correct.

[7]

+                decorator=lambda row: bugtask_decorator(row[0])

s/=/ = /

Ah, this means that the rule in [6] should be modified to be:

resultrow must be BugTask, a column of BugTask, or a sequence whose
  first element is either of the above.

It might be worth adding an assert to that.

[8]

+        undecorated = kw.get('undecorated', False)
...
+            if prejoins:
+                decorator=lambda row: bugtask_decorator(row[0])
+            else:
                 decorator = bugtask_decorator
...
+            if undecorated:
+                return resultset
             return DecoratedResultSet(resultset, result_decorator=decorator)
...
+        if undecorated:
+            return result
         return DecoratedResultSet(result, result_decorator=decorator)

I think the undecorated argument is a slightly ugly wart. Instead,
consider always returning the DecoratedResultSet, and change
searchBugIds() to:

return self._search(BugTask.bugID, [], params).result_set

[9]

+            # We need to process subscriptions, so pull all the
+            # subscribes into the cache, then update recipients
+            # with the subscriptions.

s/subscribes/subscribers/

[10]

+class ProjectGroupAndDistributionTests:
+    """Tests which are useful for project groups and distributions."""

These are about verifying distinct results. Consider renaming and
changing the docstring to reflect that.

[11]

+    BugTaskSearchParams instancs, while BugTaskSet.searchBugIds()

s/instancs/instances/

review: Approve (code)

« Back to merge proposal