Do

Code review comment for lp://staging/~alexlauni/do/package-manager-service

Revision history for this message
Chris Halse Rogers (raof) wrote :

 review needsfixing

Is this a merge for both PluginManagerService and PackageManagerService?
If so, could we merge PluginManagerService separately, and then merge
PackageManagerService after that has landed?

You've added Do.Linux.JoliCloud/Makefile.in to bzr. That's not very
useful :).

In
Do.Platform.Linux.JoliCloud/src/Do.Platform/Do.Platform.Linux/Do.Platform.Linux.JoliCloud/PackageManagerService.cs:

I don't much like the magical Daemon property, particularly since you
need to document it with the comment
"""
// this call will instantiate the daemon, as well as make sure we also
got a DBus object
"""
in Initialize (). Is there any particular reason that Initialise can't
initialise the daemon explicitly?

In MaybeStartDaemon (), is it possibly to ask DBus to wait for an owner
of BusName rather than just sleeping for 5 seconds? I'm not sure if it
is, but I'd find it cleaner.

In Do.Platform/src/Do.Platform/AbstractPackageManagerService.cs:
MaybePluginForPackage (string): It might be worthwhile extending the
plugin API to allow a plugin itself to provide a list of packages that
it supports, as well as this magic name/description matching.

What happens if multiple addins support a new package?

This comment from PluginManagerService.cs seems wrong?
/// If this class loads, we have a serious plugin because that probably
means we have no plugin manager.

review: Needs Fixing

« Back to merge proposal