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

+#: 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