[Libosinfo] [PATCH 0/6] Simplify application of datamaps

[Daniel P. Berrange]

On Wed, Jan 09, 2013 at 11:02:14AM -0500, Christophe Fergeau wrote:
> 
> 
> ----- Исходное сообщение -----
> > This is all complete overkill - the InstallScript class already has
> > info about the datamaps and can apply them to the original
> > InstallConfig
> > object instance it has, without needing to create a throwaway copy.
> > 
> > This series reverts the last 5 patches in that quoted series and
> > applies one simpler patch at the end which does the same job without
> > needing to directly associate the InstallConfig & InstallScript
> > classes at all.
> 
> While having an OsinfoInstallConfigParams property in the
> OsinfoInstallConfig class, it imo makes sense to have it as
> the disconnect between OsinfoInstallConfig and OsinfoInstallConfigParams
> is weird. It's more convenient to have the configuration schema
> (OsinfoInstallConfigParams) directly available while filling an
> OsinfoInstallConfig object, and would potentially allow in the
> future to check which parameters are allowed/disallowed/... when
> they are set.

If you're filling in a InstallConfig object you'll have to have
an InstallScript object around already, so you have the schema
available via that object if you really need it, without needing
to duplicate it in the InstallConfig object. I think it makes
the InstallConfig object design more futureproof if we don't
uneccessarily tie it to a InstallScript instance.

> As for the throw-away copy, I'm also all for getting rid of it, it
> was added after I got an objection during review. The code was initially
> doing a straight g_object_set(config, "config-params", params, NULL);
> during the generation of the install script instead of a copy.

I think that would be a bad - as a user of this API, I'd really not be
expecting it to modify the InstallConfig instance I pass in - it should
be considered 'const' IMHO.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|