Code review comment for lp:~bac/launchpad/accordion-client-2

Revision history for this message
Benji York (benji) wrote :

This branch looks good. I just have a few small comments.

I like the substitution of a function for the mix-in.

> ./lib/lp/bugs/templates/bugtarget-bugs.pt
> 169: not well-formed (invalid token)

The above can be fixed by replacing the ampersands with "&".

> ./lib/lp/registry/browser/tests/test_subscription_links.py
> 295: E301 expected 1 blank line, found 2
> 387: E301 expected 1 blank line, found 2
> 481: E301 expected 1 blank line, found 2
> 567: E301 expected 1 blank line, found 2

These are indeed spurious, but are caused by the commented-out
functions. Looking at the functions it seems that they should be
removed or made to work. I tried a little to get them to work without
luck.

The DistributionSourcePackageActionMenu class now has a mutable class
attribute ("links"), which of course is normally a bad thing but it
doesn't appear that it is ever accessed because there is a property of
the same name. I suggest just removing the class attribute altogether.

The snippet

    use_advanced_features = getFeatureFlag(
        'malone.advanced-structural-subscriptions.enabled')
    if use_advanced_features:
        links.append('subscribe_to_bug_mail')
    else:
        links.append('subscribe')

is repeated so often I wonder if a helper function named something like
"add_subscribe_link" might not be DRYer.

In these files:

    lib/lp/bugs/templates/bug-subscription-list.pt
    lib/lp/bugs/templates/bugtarget-bugs.pt
    lib/lp/bugs/templates/bugtarget-subscription-list.pt
    lib/lp/registry/templates/distribution-index.pt
    lib/lp/registry/templates/distributionsourcepackage-index.pt
    lib/lp/registry/templates/distroseries-index.pt
    lib/lp/registry/templates/milestone-index.pt
    lib/lp/registry/templates/product-index.pt
    lib/lp/registry/templates/productseries-index.pt

I think you want a "var" on the line:

    module = Y.lp.registry.structural_subscription;

review: Approve (code)

« Back to merge proposal