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

[Christophe Fergeau]

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

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libosinfo/attachments/20121218/d39fb14e/attachment.sig>