ubiquity

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

plugins
Merge into trunk

Revision history for this message

Michael Terry (mterry) wrote on 2009-09-01:

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

No, that must have been an accident from a po/real-po merge.

> I'd go for /usr/lib/ubiquity/plugins/

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

Good point. Plugins should probably go near the end, since nothing can really depend on them anyway.

> 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?

I didn't like doing that, because I didn't want to collide with future extensions. That is, I don't want a PageGTK to define a convenience member blarg today, if we may start using blarg as part of the API tomorrow. I suppose that's an argument for versioning.

However, it's not like I really avoided that problem with the current design. Right now, there are 3 interactions -- plugin->frontend-methods-and-data (Controller), frontend->plugin-methods (just calls the method on the PageGtk), and frontend->plugin-data (get_ui()). So we already have the collision problem with methods on the PageGtk.

We could fix this by a reverse Controller, putting methods pointers in get_ui() as well, namespacing API PageGtk members, or namespacing non-API PageGtk members.

I think I probably like namespacing non-API members the best and getting rid of get_ui(), as you suggest. Then official recommendations to plugin writers would be that all convenience members should start with an underscore.

> 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?

Nothing, now that you say it; I used isinstance elsewhere. Not sure what made me be so clunky there.

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

No, that must have been an accident from a po/real-po merge.

> I'd go for /usr/lib/ubiquity/plugins/

Good point.  Plugins should probably go near the end, since nothing can really depend on them anyway.

> 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?

I didn't like doing that, because I didn't want to collide with future extensions.  That is, I don't want a PageGTK to define a convenience member blarg today, if we may start using blarg as part of the API tomorrow.  I suppose that's an argument for versioning.

However, it's not like I really avoided that problem with the current design.  Right now, there are 3 interactions -- plugin->frontend-methods-and-data (Controller), frontend->plugin-methods (just calls the method on the PageGtk), and frontend->plugin-data (get_ui()).  So we already have the collision problem with methods on the PageGtk.

We could fix this by a reverse Controller, putting methods pointers in get_ui() as well, namespacing API PageGtk members, or namespacing non-API PageGtk members.

I think I probably like namespacing non-API members the best and getting rid of get_ui(), as you suggest.  Then official recommendations to plugin writers would be that all convenience members should start with an underscore.

> 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?

Nothing, now that you say it; I used isinstance elsewhere.  Not sure what made me be so clunky there.

« Back to merge proposal