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

[Zach Welch]

On 02/25/2015 08:43 AM, Dan Smith wrote:
>> +#: 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'

That method maps the number to the name.  I don't see a method that maps 
from name to number, which is what that dict does for us.

>> +        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.

Fixed.

>> +                try:
>> +                    level = log_evel_names[CHIRP_DEBUG]
>
> Typo here: log_evel_names doesn't match above.

Good catch.  Also, the matching except for this needs to catch KeyError.

>> +                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.

With proper logging, we can start adding many more LOG.<level>() calls, 
which may make it desirable to limit the output.  Basically I think that 
it's a feature that I will use, or I wouldn't have added it.

...
> 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.

There is nothing stopping us from always logging to such a location in 
addition to other places.

Thus, I would vote for adding a new handler for that purpose, rather 
than changing the existing code (which gives developers more 
flexibility).  The full DEBUG level output is really too verbose, and I 
would be annoyed if that were my only option.

I also vote to punt on that addition for another patch.

>> +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?

I can imagine adding a menu option to the GUI ("Enable logging") and a 
window that updates in real-time.  Naturally, that would be down the 
road, but that would be a wickedly useful feature for users.  They could 
just copy and paste the relevant log information straight out of the GUI 
and into their mailer.  Thus, I say the Logger class should be left 
intact, though I will move the LOG variable into the class.

>> -                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.

Yeah, okay...  It was more for my own testing than anything.  I'll 
change it back.

-- 
Zach Welch
Mandolin Creek Farm
www.mandolincreekfarm.com
farm: 541-453-4131
cell: 541-740-3410