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

[Zeeshan Ali (Khattak)]

On Wed, Dec 19, 2012 at 12:33 AM, Christophe Fergeau
<cfergeau at redhat.com> wrote:
> On Tue, Dec 18, 2012 at 08:30:16PM +0200, Zeeshan Ali (Khattak) wrote:
>> On Tue, Dec 18, 2012 at 8:15 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> > Actually, OsinfoInstallScript does not expose an
>> > OsinfoInstallConfigParamList as this class is introduced by this series.
>> > Not having this class in the first place is one of the reasons I assumed
>> > this part of the code may need some improvements...
>>
>> I did indeed forget that fact that the list itself is not being
>> exposed as is but rather InstallScript only provides methods to access
>> params in the list.
>
> The list is exposed, but as a GList, not as an
> OsinfoInstallConfigParamList, which is what would be consistent with the
> rest of the libosinfo API. Part of this series fixes that by adding
> an OsinfoInstallConfigParamList class and making use of it.
>
> If something makes some code redundant in this series, and could cause some
> of the osinfo_install_script_*_config_param_* methods to become obsolete,
> it's this change ("OsinfoInstallScript: Use an OsinfoInstallConfigParamList").
>
> And this one really is a worthwhile cleanup.
>
>>
>> > We have various methods on the OsinfoInstallScript class to handle
>> > OsinfoInstallConfigParam, but they look out of place to me, especially
>> > after the introduction of the OsinfoInstallConfigParamList class. I
>> > wouldn't mind deprecating these methods, they are probably not widely used
>> > anyway.
>>
>> None of the libosinfo API is yet widely used. The main app that uses
>> libosinfo is Boxes and we are using the methods in question in there.
>
> Fwiw, I could find one single use of osinfo_script_get_config_param, so
> whatever happens, it should not be too hard to adjust...
>
>>
>> As I said before, I don't think there is a compelling reason to
>> deprecate APIs in this case. Hence the reason I wanted Daniel to make
>> the decision here.
>
> You are the one saying we should deprecate some methods, and at the same
> time saying we should not be deprecating things.

I've really had it with you generalizing my statements and taking them
out of context.

I don't think we should be deprecating APIs without a good reason. In
this case, I don't see any.

> As far as I'm concerned, after applying the whole patch series, the old API
> is still working as always, I don't see reasons for it being broken any
> time soon, and as far as I'm concerned the old API can stay.

As I said twice already, that will mean confusing the app developer
with two competing APIs.

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

> I'd just like that we export osinfo_install_config_new_for_script and
> promote its use in our docs as imo this will be useful moving forward, but
> that's it.

Not deprecating existing API (now) while adding a competing API to it
and promoting that in the docs is no solution. Its like deprecating
API silently.

Once again, I'll propose that Daniel makes the decision here. I don't
know why you keep ignoring that suggestion.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124