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

[Zeeshan Ali (Khattak)]

On Wed, Jan 9, 2013 at 12:49 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 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.

Actually it was you who mentioned 'path-format' on
OsinfoInstallConfig being a good idea and gave an example of how it
will be useful to Boxes in this thread and you presented that as an
argument in favor of your patch.

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

Well, I think Daniel got it sorted out now so we can stop arguing on
these obsolete patches. :)

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124