[chirp_devel] [PATCH 6/9] Add chirp.logger module (#2347)

[Dan Smith]

> +#: Map human-readable logging levels to their internal values.
> +log_level_names = {"critical": logging.CRITICAL,
> +                   "error":    logging.ERROR,
> +                   "warn":     logging.WARNING,
> +                   "info":     logging.INFO,
> +                   "debug":    logging.DEBUG,
> +                   }

I don't think we need this:

  >>> logging.getLevelName(logging.DEBUG)
  'DEBUG'

> +
> +
> +class Logger(object):
> +    def __init__(self):
> +        # create root logger
> +        self.logger = logging.getLogger()
> +        self.logger.setLevel(logging.DEBUG)
> +
> +        self.LOG = logging.getLogger(__name__)
> +
> +        # Set CHIRP_DEBUG in environment for early console debugging.
> +        # It can be a number or a name; otherwise, level is set to 'debug'
> +        # in order to maintain backward compatibility.
> +        CHIRP_DEBUG = os.getenv("CHIRP_DEBUG")
> +        level = logging.WARNING
> +        if CHIRP_DEBUG:
> +            try:
> +                level = int(CHIRP_DEBUG)
> +            except:

I know the chirp code is not a good example of this, but using "except
exception" is generally frowned upon. ValueError is the thing you're
trying to catch here, so you should just catch it.

> +                try:
> +                    level = log_evel_names[CHIRP_DEBUG]

Typo here: log_evel_names doesn't match above.

> +                except:
> +                    level = logging.DEBUG

IMHO, CHIRP_DEBUG is really just a hack we put in place in a few places.
If it's set, I'd set logging to debug and if not, do nothing.

> +
> +        self.console = logging.StreamHandler()
> +        self.console.setLevel(level)
> +        format_str = '%(levelname)s: %(message)s'
> +        self.console.setFormatter(logging.Formatter(format_str))
> +        self.logger.addHandler(self.console)
> +
> +        # Set CHIRP_LOG in envoronment to the name of log file.
> +        logname = os.getenv("CHIRP_LOG")
> +        self.logfile = None
> +        if logname is not None:
> +            self.create_log_file(logname)
> +            level = os.getenv("CHIRP_LOG_LEVEL")
> +            if level is not None:
> +                self.set_log_verbosity(level)
> +            else:
> +                self.set_log_level(logging.DEBUG)
> +
> +        self.LOG.debug("logging started")

A huge part of our ability to support people when they file bugs is
asking them for the debug log. On Windows and MacOS, that is in chirp's
profile directory, called 'debug.log'. IMHO, that's not something we
want to lose.

So, I think this always needs to log at debug level to that location. If
you want to add an option to disable it, or make it conditional on
istty() of stdout, then that's fine too.

Given that, I'm not sure it's important to have an other configurable
log, but if it is, I think doing so via an argument is sufficient. It's
"really hard" for Windows people to set an environment variable.

> +LOG = Logger()

IMHO, module-scope variables named "LOG" should be instances of the
logging.Logger class. I don't really think the stuff you have in this
class needs to be a class, but I also don't think it needs to be
referenced outside of this module. I think we can just setup logging at
start time and be done with it, right?

> -                print "Must specify a radio model"
> +                LOG.critical("You must specify a radio model.")
>                  sys.exit(1)

I don't think it makes sense for this to be a log call, does it? It's
usage information printed before we do anything.

Otherwise, really happy to see us moving towards some organized logging
of things, thanks!

--Dan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
Url : http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20150225/f4f2988e/attachment-0001.bin