[Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[Rob Crittenden]

John Dennis wrote:
> On 07/20/2012 12:34 PM, Petr Viktorin wrote:
>> On 07/20/2012 05:59 PM, John Dennis wrote:
>>>>> A fair amount of the code in the framework is doing this now, but the
>>>>> install code was never cleaned up. That was left for another day, I
>>>>> guess that day is here.
>>>>
>>>> Updated. I also added the necessary lint exception.
>>>> I'm curious as to why it works that way, though. Normally, to put
>>>> methods in a class, you would use a base class/mixin. Why this dynamic
>>>> magic?
>>>
>>> Good question. I don't like dynamic magic either, it makes static code
>>> reading difficult and diminishes the value of static code
>>> analysis/checking. Jason who originally wrote the framework used dynamic
>>> magic a fair amount, including the initialization of the logging methods
>>> in the plugins. When I wrote the log manager I kept Jason's existing
>>> strategy (how many things do change at once?). In hindsight a mixin
>>> would have been a better solution, something we should probably fix down
>>> the road.
>>
>> Thinking more about this, composition would probably be cleaner than
>> inheritance here (i.e. using self.logger.warn(...) instead of mixing the
>> functionality into the class itself).
>> Well, one day the time to fix it will come.
>>
>>> Everything looks good, although there might be one minor thing that
>>> needs fixing. Shouldn't the logging setup be done first? That way any
>>> code that executes has the ability to emit a message. For example what
>>> if validate_options() wants to emit a message?
>>>
>>
>> That's a good question.
>> If we set up logging before validating the options, we'll log all
>> invocations, including those with misspelled option names and forgotten
>> "sudo"s. Do we want that?
>>
>
> Sure, hard to say. Why don't we leave it the way it is now and down the
> road if it shows up as an issue we'll tweak it then.
>
> ACK.
>

pushed to master