Code review comment for lp:~bac/launchpad/bug-643538-code

On Sep 23, 2010, at 11:49 , Gavin Panella wrote:

> Review: Approve code
> Cool branch :)
>
> The comments I have are all really minor.

Thanks for the helpful review Gavin.

>
> Gavin.
>
>
> [1]
>
> def getDistroDevelSeries(self, distribution):
> - """Since distribution.currentseries hits the DB every time, cache it."""
> + """distribution.currentseries hits the DB every time so cache it."""
> self._distro_series_map = {}
> try:
> return self._distro_series_map[distribution]
> except KeyError:
> result = distribution.currentseries
> self._distro_series_map[distribution] = result
> return result
>
> I think your change doesn't describe what the code does... but that's
> because the code is broken! The following line is the culprit:
>
> self._distro_series_map = {}
>
> Every time the method is called the cache gets reset. I think this
> line can be removed.

Thanks for looking. My change was just to stop lint from complaining about the comment being too long -- I didn't even see the broken code.

>
>
> [2]
>
> + self.branch = self.development_focus_branch
>
> development_focus_branch is a cachedproperty, so perhaps branch should
> be a property so that development_focus_branch is not computed until
> it is needed.
>

Good catch.

>
> [3]
>
> + self.assertEqual(None, find_tag_by_id(contents, 'branch-portlet'))
> ...
> + self.assertNotEqual(None, find_tag_by_id(contents, 'branch-portlet'))
>
> I think this (and others that test against None) should be done with
> assertIs(None, ...). Can't remember what difference is makes though :)
>

Done

>
> [4]
>
> + def test_portlets_shown_for_EXTERNAL(self):
> + # If the BranchUsage is HOSTED then the portlets are shown.
>
> s/HOSTED/EXTERNAL/
>

Done

>
> [5]
>
> + <a tal:attributes="href view/mirror_location"><tal:mirror replace="view/mirror_location"/></a>.
>
> Doesn't really matter, but it might be clearer expressed as:
>
> <a tal:attributes="href view/mirror_location"
> tal:content="view/mirror_location"/>.
>
>

Yes, much nicer.

> [6]
>
> + <div tal:condition="not:view/new_branches_are_private" id="privacy-text">
> + <p>
> + New branches you create for <tal:name replace="context/displayname"/>
> + are <strong>public</strong> initially.
> + </p>
> + </div>
> + <div tal:condition="view/new_branches_are_private" id="privacy-text">
> + <p>
> + New branches you create for <tal:name replace="context/displayname"/>
> + are <strong>private</strong> initially.
> + </p>
> + </div>
>
> The tal:condition could go on the subordinate <p> tags, then there's
> only one <div>.
>

Done.

>
> [7]
>
> + <tal:comment condition="nothing">
> + The view/*_count|nothing expressions below are so that this
> + template can be rendered by a view that does not have count
> + information available.
> + </tal:comment>
>
> There aren't any view/*_count|nothing expressions, so this comment can
> go.
>
>
c-n-p error

> [8]
>
> + configured = sum([1 for v in config_statuses.values() if v])
>
> Don't need the list comprehension.

I always throw those in. Yanked.