[Fedora-livecd-list] [PATCH] Make use of python logging API for debug messages

[Jeremy Katz]

This looks pretty good in general.  But a few comments/questions.  Most
of which are nitpicky little things, but there's a little bit of meat in
the middle :)

On Mon, 2008-05-19 at 11:58 -0400, David Huff wrote:
> diff --git a/imgcreate/creator.py b/imgcreate/creator.py
> index 1028e32..febb847 100644
> --- a/imgcreate/creator.py
> +++ b/imgcreate/creator.py
> @@ -522,7 +523,7 @@ class ImageCreator(object):
>                                          (pkg, e))
> 
>           for pkg in skipped_pkgs:
> -            print >> sys.stderr, "Skipping missing package '%s'" % (pkg,)
> +            logging.info("Skipping missing package '%s'" % (pkg,))

Maybe should be logging.warn?

> @@ -537,7 +538,7 @@ class ImageCreator(object):
>                       skipped_groups.append(group)
> 
>           for group in skipped_groups:
> -            print >> sys.stderr, "Skipping missing group '%s'" % 
> (group.name,)
> +            logging.info("Skipping missing group '%s'" % (group.name,))

And the same.

> +def handle_logfile(option, opt, val, parser, logger, stream):
> +    try:
> +        logfile = logging.handlers.RotatingFileHandler(val, "a",
> +                                                       LOGFILE_MAX_BYTES,
> +                                                       LOGFILE_N_BACKUPS)

A rotating log file seems like overkill here.  Let's keep it just a
simple file.  

> +    logfile.setFormatter(logging.Formatter(FILE_FORMAT, DATE_FORMAT))

Is there really a convincing reason not to just stick with the default
format handling?

> +def setup_logging(parser = None):
> +    """Set up the root logger and add logging options.

What happens if the utility doesn't call setup_logging()?  It's
important that the behavior be basically the same for anyone who has
built something on top of the existing API who doesn't immediately go
and add the setup_logging() call

> diff --git a/tools/livecd-creator b/tools/livecd-creator
> index 7c08323..dddfddd 100755
> --- a/tools/livecd-creator
> +++ b/tools/livecd-creator
> @@ -22,8 +22,10 @@ import os.path
>   import sys
>   import time
>   import optparse
> +import logging
> 
>   import imgcreate
> +import imgcreate.debug

This import isn't needed anymore.

Jeremy