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

[Zeeshan Ali (Khattak)]

On Wed, Dec 19, 2012 at 5:43 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Wed, Dec 19, 2012 at 04:55:23PM +0200, Zeeshan Ali (Khattak) wrote:
>> On Wed, Dec 19, 2012 at 11:16 AM, Christophe Fergeau
>> <cfergeau at redhat.com> wrote:
>> > When I started working on these patches, I was very confused by
>> > OsinfoInstallConfig VS OsinfoInstallConfigParam. They have similar names,
>> > but they have no relationship whatsoever, you cannot get one from the
>> > other, you are not using them in the same API calls, ... so the API is
>> > confusing anyway.
>>
>> I understand and agree that this API is confusing but by keeping it
>> and adding the same API on another class will cause a lot more
>> confusion. That would be actually contrary to what you are trying to
>> achieve here.
>
> I don't know what you are talking about here, use specific method and class
> names and make a detailed answer. We don't seem to be talking about the
> same thing.
>
>>
>> >> > After looking at what Boxes does, it seems to make sense to have the
>> >> > OsinfoInstallConfigParamList be available from both
>> >> > OsinfoInstallScript and OsinfoInstallConfig depending on what is more
>> >> > convenient for the user at a given time.
>> >>
>> >> How would one be more convenient than the other?
>> >
>> > If you have an OsinfoInstallScript object because you are setting up
>> > 'stuff' which you'll then use to create OsinfoInstallConfig(s), then it's
>> > more convenient to get the OsinfoInstallConfigParamList from the script as
>> > you haven't started creating your config. This is the way Boxes is using
>> > it.
>> >
>> > If you have already started your OsinfoInstallConfig and need to know which
>> > parameters are mandatory, which are supported, ...
>>
>> That is dependent on the script and config alone can not tell you
>> that. The only way config can tell you this is to get this information
>> from a script.
>>
>> > then it's more
>> > convenient to get the OsinfoInstallConfigParamList from the config as you
>> > may not have as script handy.
>>
>> You can't have this information without a script instance at hand for
>> the reason I mentioned above. So I'm sorry but I don't see any
>> convenience being added.
>
> This avoids having to carry around an OsinfoInstallScript instance when all
> you are doing is filling your OsinfoInstallConfig object, you don't have to
> pass it through every method calls, ...

If I understood your current patch series correctly,
OsinfoInstallConfig:config-params is not set before app generates the
script so:

1. by the time OsinfoInstallConfig:config-params is set,
OsinfoInstallConfig is unlikely to be useful to the app anymore.
2. it would be totally unintuitive for app to have access to
OsinfoInstallConfig:config-params only after generating the script.

Now if you want to revisit the decision to make
osinfo_install_config_new_for_script() private, that is another thing.
Otherwise, I really don't see what you want to achieve with exposing
OsinfoInstallConfig:config-params in public API.

> Had 'path-format' described as an OsinfoInstallConfigParam (which seems to
> make sense from a quick glance), Boxes would even have been able to benefit
> from that in UnattendedInstaller::configure_install_script, the
> OsinfoInstallScript argument would not have been needed.

Except that it wouldn't have worked with your current patches for the
reason I mentioned above.

>> > What I'm suggesting complements and improves the
>> > existing API, it does not really compete with it.
>>
>> So I take it that you have changed your opinion slightly on this cause
>> you were the first to call these two approaches "competing" in this
>> thread?
>
> Well, I don't know what I used to be saying, but yeah I've said several
> times in this email that my thinking on all of this was still evolving.

Within this thread, I've seen your thinking evolving only in regards
to justification of your patches. First you were of the opinion that
InstallConfig:param-list is a 'competing approach' to config param API
of OsinfoInstallScript and we should move towards the former approach.
Then you changed the opinion to: These two are complementary to each
other and app can choose which one to use depending on the scenerio.
Thats what I gathered and I could be totally wrong so feel free to
correct.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124