Code review comment for lp:~mterry/ubiquity/plugins

Revision history for this message
Colin Watson (cjwatson) wrote :

In general I like this approach. The branch is much too complicated for me to be able to do a line-by-line, so I'm just going to make some general comments and trust that you've done industrial-strength testing. In particular, noting your locale changes, I strongly recommend testing in Portuguese (both Portugal and Brazil) and Chinese.

Any particular reason that po/ubiquity.pot is removed in this branch?

I'd go for /usr/lib/ubiquity/plugins/ rather than /usr/lib/ubiquity/plugins.d/. The reason for this is that, normally, foo.d directories are a split-out analogue of a corresponding single file (see e.g. /etc/apt/sources.list and /etc/apt/sources.list.d/). In this case, there's no corresponding single file, and it would make more sense to drop the ".d".

There are some delicate dependencies in scripts/install.py which I'm not entirely sure you've preserved. The main one that comes to mind is that configure_timezone (scripts/install.py) may require network access for NTP syncing. I don't think configure_network has delicate dependencies of its own so you can probably just run it earlier.

Please always put a blank line between method definitions, even if the methods are short.

I'm not sure I like the get_ui design. Wouldn't it be simpler to just have attributes on the PageGtk etc. instances directly? Then the simple case would just be something like:

  class PageGtk:
      optional_widgets = 'stepMigrationAssistant'

... while in more complicated cases you could initialise attributes in __init__. You can then use hasattr() to find out if a page instance has a particular attribute, if need be.

Unless I'm missing something subtle, you should omit blank __init__ methods from page classes.

Could PageGtk (etc.) instances inherit from something, so that you have a debug method similar to that in FilteredCommand? That would be better than printing directly to sys.stderr in places.

"if type(widget_list).__name__ != 'list'" etc. - what's wrong with isinstance?

review: Needs Fixing

« Back to merge proposal