Code review comment for lp:~wallyworld/launchpad/built-packages-listing

Thanks for the review.

>
> There are a lot of classes and methods that don't have docstrings. Even a
> short docstring is often more useful than nothing.
>

Done.

> PEP-8 says a blank line between class definition line and methods.
>
> sorted(builds, key=attrgetter('id')) is a way to avoid the zope proxy issue
> and sort on something that is unique and has a __lt__ implementation.
>

RecipeBuildRecord, being a named tuple of entities, doesn't have an id
attribute so this suggestion is not relevant here.

> expected_text = "No recently completed daily builds found."
>
> is more readable IMO than: (also PEP-8)
>
> expected_text = """
> No recently completed daily builds found.
> """

Indeed.

>
> In the bazaar doctest, instead of using browser.goBack(), just open the page
> directly. This way I don't have to know where the browser was before.
>

The original test was written using browser.goBack() so I was extending
what was there already. I've implemented the suggestion and as a drive
by fixed the other places where goBack() was used.

> You want to avoid string concatenation in the TAL:
> tal:attributes="href
> string:${dailybuild/recipebuild/distribution/fmt:url}/+source/${dailybuild/sourcepackagename/name
>
> This should be change to be a method somewhere either in the view or the items
> in the result set. That way it would just be:
>
> tal:attributes="href xxx/distro_source_package/fmt:url"
>
>

My original implementation was lifted exactly from:
 question-portlet-details.pt
I've reworked it as per the suggestion by adding a method to the
RecipeBuildRecord class