Code review comment for lp:~software-updates-spec/update-manager/group-by-applications

Michael Vogt (mvo) wrote :

On Tue, Jan 22, 2013 at 06:25:25PM -0000, Michael Terry wrote:
> Michael Terry has proposed merging lp:~software-updates-spec/update-manager/group-by-applications into lp:update-manager.
>
> Requested reviews:
> Ubuntu Core Development Team (ubuntu-core-dev)
>
> For more details, see:
> https://code.launchpad.net/~software-updates-spec/update-manager/group-by-applications/+merge/144362
>
> This is a continuation of https://code.launchpad.net/~dylanmccall/update-manager/group-by-applications/+merge/112678
>
> This implements the 'Available Updates' description pane in the Software Updates spec:
> https://wiki.ubuntu.com/SoftwareUpdates#Expanded_presentation_of_updates

Thanks for this branch! I haven't really had a chance to test-run it
yet, my bandwidth right now is not very good. From looking at the diff
it looks good, I like the new tests and the new
"aptroot-group-testing"!

Some things I noticed during reading the diff, nothing that really
needs fixing, just tiny nit-picking.

[..]
> + def __init__(self, parent, dist=None):
> + self.dist = dist
> + if self.dist is None:
> + try:
> + self.dist = subprocess.check_output(
> + ["lsb_release", "-c", "-s"],
> + universal_newlines=True).strip()
> + except subprocess.CalledProcessError as e:
> + print("Error in lsb_release: %s" % e)
[..]

As a tiny nit-pick, this above could be written also via:

import platform
self.dist = platform.dist()[2]

[..]
> +import xml.sax.saxutils
..
> +def get_package_label(pkg):
> + """ this takes a package synopsis and uppercases the first word's
> + first letter
> + """
> + import xml.sax.saxutils
> + name = xml.sax.saxutils.escape(getattr(pkg.candidate, "summary",
> ""))
> + return capitalize_first_word(name)

Looks like one of the imports can be skipped. Or (probably better) we use
GLib.markup_escape_text() instead.

> + if len(cells) != 3 or \
> + not isinstance(cells[0], Gtk.CellRendererToggle) or \
> + not isinstance(cells[1], Gtk.CellRendererPixbuf) or \
> + not isinstance(cells[2], Gtk.CellRendererText):
> + return

Most of the other python code seems to put "(" around ")" the multi
line if statements, either is fine with me, just wanted to mention
it.

Cheers,
 Michael

« Back to merge proposal