[Libosinfo] [PATCHv4 08/11] Use OS-specific config in OsinfoInstallScript

[Zeeshan Ali (Khattak)]

On Tue, Dec 18, 2012 at 6:19 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Tue, Dec 18, 2012 at 04:45:15PM +0200, Zeeshan Ali (Khattak) wrote:
>> On Tue, Dec 18, 2012 at 12:05 PM, Christophe Fergeau
>> <cfergeau at redhat.com> wrote:
>> > Yup, I've thought of both, we can also do if (config == entity) {}, but I
>> > preferred to go the explicit way since the caller knows what it wants us to
>> > do.
>>
>> Caller knows what it is passing and callee has easy ways to find out
>> the type so why would you need this extra parameter? You are not
>> making anything more explicit either in the caller or callee
>> functions. Sorry but this just looks very clumsy.
>
> Callee has an easy way to _guess_ how it was called by harcoding checks for
> something that is true the way the code is _now_, this assumption could
> easily break with some future changes.

I don't see how and where is the hardcoding? If in future Config
instance is not passed, the passed param will never be TRUE. In the
absence of that param, everything will work in the same way.

> I'm fine with changing to a runtime
> type check, but I don't see this as an improvement, just clumsy in a
> different way.

You need to check the type and you use a macro that is provided for
exactly that purpose. There is nothing clumsy about that. Whereas what
you are doing is something I haven't seen before.

The ideal case would be to not have to check the type at all but
rather have a specialized function for dealing with Config entities.
You can do it that way too.

I have suggestion for even better solution but that would mean more
work that we can live without (for now).

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124