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

Cool branch :)

The comments I have are all really minor.

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.

[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.

[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 :)

[4]

+ def test_portlets_shown_for_EXTERNAL(self):
+ # If the BranchUsage is HOSTED then the portlets are shown.

s/HOSTED/EXTERNAL/

[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"/>.

[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>.

[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.

[8]

+ configured = sum([1 for v in config_statuses.values() if v])

Don't need the list comprehension.