[Freeipa-devel] [PATCH 0072] Provide ipa-client-advise tool

[Jan Cholasta]

On 21.6.2013 11:45, Tomas Babej wrote:
> Newly added features:
>
>   - options propagated to plugins
>   - made plugin content creation more comfortable, now 3 classes of
> output are
>     available (debug, comment, command)
>
> Now pretty much everything that comes into my mind is addressed, so
> please have a look
> at the current implementation.

The patch needs a rebase.

+    class AdviceLogger(object):

Please don't use nested classes. If you want AdviceLogger to be 
private-ish, you can rename it to _AdviceLogger.

Also I think AdviceLogger is a little bit misleading name, I would 
prefer AdviceOutput or something like that.

Functionally the patch is OK, but I have some second thoughts about the 
design. I'm not sure if using API plugins is truly the right thing to 
do, as advises seem to be pretty much orthogonal to the rest of our API. 
There are some negative side effects, such as initializing the API every 
time ipa-advise is run, for each and every advice, which takes some 
time, so there is a short but noticable delay. What are the benefits of 
using API plugins for this, besides code reuse? (I'm not saying this 
must be changed, just give it some thought, using something simpler 
might be better.)

Honza

-- 
Jan Cholasta