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

[Zeeshan Ali (Khattak)]

On Tue, Dec 18, 2012 at 7:17 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Tue, Dec 18, 2012 at 07:09:06PM +0200, Zeeshan Ali (Khattak) wrote:
>> 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.
>
> We are hardcoding the fact that we want to transform parameters values
> for all OsinfoInstallConfig instances that are passed to this function,
> which may or may not be true depending on how this code evolves. Anyway,
> changed already.
>
>> > 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.
>
> I don't really want to check the type, I want to know if I should try to
> transform the values or not. Agreed, the variable was badly named for what
> I had in mind with this check.
>
>
>> I have suggestion for even better solution but that would mean more
>> work that we can live without (for now).
>
> Well, that's still interesting even if we don't do it..

OK but don't think of it as something I want you to implement. :) The
idea was simply to move this function to Entity class as virtual (and
internal) and then have Config override it.. but now that I said it, I
just realized that would be impossible to implement without breaking
ABI. :(

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124