Code review comment for lp:~wallyworld/launchpad/sharing-details-delete-966641

Thanks for the review.

On 04/04/12 01:58, Curtis Hovey wrote:
> Review: Approve code
>
> I see the SharingDetailsTable.initializer lost its bugs and branches setters, yet the tests show everything works. What this unused data? I think the bugs and branches in the cache were the real data source, not the config.
>

The initialiser doesn't need the setters - because bugs and branches are
declared as attributes, if there are present in the config passed to the
initialiser, the attributes are set automatically. So the code that was
removed as totally redundant.

The view/controller grabs the bugs and branches from the data model
(json cache) and uses these to initialise the widget.

> Line 355 repeats the common mistake of creating a sprite without allocating space for it. Elements with sprites must contain content, add a nbsp;
> <span class="sprite bug-{{bug_importance}}">&nbsp;</span>',
>

Right. Missed fixing that one. Sorry.

> Line 529 has bogus markup "<br><br>" never use two <br />, because more than one means your markup and css is broken. Most browser strip all but the first one when rendering sibling blocks because they know css is the correct way to handle the situation. the <br /> must state it is empty as well. I can see the style attr is used with a class. This markup is just broken. I am certain it came from somewhere else in Lp.
>

Will fix.

> This is a small semantic issue I share with Julian, "should" implies we are uncertain about how the code we wrote operates. "Should" on lines 729 and 999 adds a Heisenberg/Wittgenstein state of uncertainty, when I am very certain the module "must" be loaded for the test to complete. Maybe
> "Could not locate the "

The text in question came from the test template that Rick did. I'll fix
the text here and in the template.