[Libosinfo] [PATCHv4 06/11] Add OsinfoInstallConfig:config-params property

[Christophe Fergeau]

Hey,

----- Исходное сообщение -----
> On Thu, Dec 20, 2012 at 6:45 PM, Christophe Fergeau
> <cfergeau at redhat.com> wrote:
> > This property lists the parameters that can be set for a given
> > OsinfoInstallConfig. This is not enforced, it's only there for
> > informative purpose. This will also be used in later commits
> > in order to automatically apply transformations on values
> > for parameters which have an associated OsinfoDatamap.
> 
> Sorry to revive this thread again but I had an idea that I wanted to
> discuss before this API goes into a release: How about we have a
> OsinfoInstallConfig:install-script (of type OsinfoInstallScript)
> rather than having OsinfoInstallConfig:config-params (and
> 'path-format', 'avatar-format' etc) duplicated on both objects?

I don't understand your 'path-format', 'avatar-format', ... comment. The OsinfoInstallConfig::config-params property appears on both OsinfoInstallConfig and OsinfoInstallScript, but the actual OsinfoInstallConfigParams instance is shared between these 2 classes, so not much duplication either.

>IMO
> that makes a lot more sense since that not only avoids duplication of
> API (and some strings) but also makes things more clear: Whether a
> config is associated/specific to a script or not? If it is, which
> install script is exactly its tied to?

In my opinion, this is completely backward. When you have an OsinfoInstallScript, it's interesting to know the OsinfoInstallConfig
that is set on it. When you have an OsinfoInstallConfig instance, it's interesting to know the schema that is valid for this config, ie to have access to the OsinfoInstallConfigParams associated with it. If anything, I'd add an OsinfoInstallScript::install-config property of type OsinfoInstallConfig, which becomes a bit harder after your suggested change if you want to avoid cyclic references.

As I see it, OsinfoInstallScript ties an OsinfoInstallConfig with an install script template, and when filling OsinfoInstallConfig, you should not need to care at all about the other stuff that OsinfoInstallScript can do, all the info you need should be in OsinfoInstallConfigParams.

So -1 on this change from me.

Christophe