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

Revision history for this message
Tim Penhey (thumper) wrote :

Additional review comments:

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

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.

expected_text = "No recently completed daily builds found."

is more readable IMO than: (also PEP-8)

expected_text = """
    No recently completed daily builds found.
    """

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.

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"

review: Needs Fixing

« Back to merge proposal