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

[Petr Viktorin]

On 07/19/2012 08:01 PM, John Dennis wrote:
> Overall I really like the approach, good work.
>
> I have not applied and run the patch, so my comments are based only on
> code reading.
>
> I have just a small things which might need changing.
>
> One of the ideas in the log manager is to easily support per class
> logging (borrowed from Java). This allows you to enable/disable or
> otherwise configure logging per logical code unit as opposed to a big
> global hammer (i.e. root logger). It also allows for the class name
> (logger name) to show up in the log output which can be really handy so
> you know which class is emitting a message (we don't currently turn this
> on but it's a trival one-line change to the log format spec.).
>
> Each significant class should initialize it's own logging, which is very
> easy to do with this line at the top of a class's __init__()
>
>      log_mgr.get_logger(self, True)
>
> so instead of:
>
>      self.logger.info()
>
> you would say:
>
>      self.info()
>
> 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?

> Also each log method (i.e. info(), warn(), error(), etc) should pass
> it's format substitutions as parameters rather than preformatting the
> message before submitting it to the logger. This is a performance
> optimization because the logger delays building the message until it
> knows the message will actually be emitted and not discarded (common
> case with debug messages).
>
> So instead of:
>
>      self.debug("the contents of this big dict is: %s" % my_dict)
>
> You would write:
>
>      self.debug("the contents of this big dict is: %s", my_dict)
>
> A small different in coding, but a lot less work at run time.

Fixed, thanks for noticing.

> I'm not sure the following message handling is optimal
>
> +        message = '\n'.join(
> +            "The hostname resolves to the localhost address
> (127.0.0.1/::1)",
> +            "Please change your /etc/hosts file so that the hostname",
> +            "resolves to the ip address of your network interface.",
> +            "",
> +            "Please fix your /etc/hosts file and restart the setup
> program",
>
>
> I'm not a big fan of individual lines of text in the source that are
> glued together or are emitted individually via a series of print
> statements. I'd rather see multi-line text as multi-line text using
> Python's multi-line string operator (e.g. ''' or """). That way you can
> see the text as it's meant to appear, but there is a bigger reason.

You're right, fixed. To keep sane indentation I used textwrap.dedent.

> Shouldn't this message be internationalized? (e.g. wrapped in _()). When
> a message is internationalized it's even more important for multi-line
> text to appear in it entirety to give translators the greatest context
> and latitude for their translation rather than arbitrarily splitting it
> up into fragments according to English norms (fragments that might not
> even translate in another language).
>
> Also, a quick glance suggests a number of the messages need i18n treatment.

None of the tools are internationalized. Perhaps they need to be, but it 
should be a separate ticket.

> Overall though, I really love the approach, great work!

Thanks for the review :)

-- 
Petr³


-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0056-06-Framework-for-admin-install-tools-with-ipa-ldap-upda.patch
Type: text/x-patch
Size: 31928 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120720/6f3089eb/attachment.bin>