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