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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
