Feature/log lvl arg parsed #51
Conversation
| log = helpers.prepareLogger("APP", os.path.join(logDir, "tinydecred.log"), logLvl=0) | ||
| log = helpers.getLogger("APP") |
There was a problem hiding this comment.
Not quite sure what you're trying to accomplish here. These functions do very different things. prepareLogger must be called before getLogger, and prepareLogger should only be called once for the application. Look at helpers.py to see how those functions work.
| for logLvlModule in cfg.logLvls: | ||
| try: | ||
| parts = logLvlModule.split(":") | ||
| module = parts[0].upper() | ||
| logLvl = helpers.getLogLevel(parts[1]) |
There was a problem hiding this comment.
This parsing should be done during configuration parsing (config.go). Make it a function so you can test it. You also don't appear to be handling the default case, --log=DEBUG, where no module is specified. That log level should be passed to helpers.prepareLogger to set the root logging level.
Read up on the logging module and how child loggers work. I would move all logger configuration to config.py, including the call to perpareLogger, and then just retrieve the loggers from the individual modules, without resetting the level. This will require modification of the getLogger function so that setLevel is not run during retrieval.
Line 227 in a903469
| module = parts[0].upper() | ||
| logLvl = helpers.getLogLevel(parts[1]) | ||
|
|
||
| helpers.prepareLogger( |
There was a problem hiding this comment.
See previous comment. This is the wrong way to use this function.
| "-l", | ||
| action="append", | ||
| type=str, | ||
| help="set logging level [module:lvl]", |
There was a problem hiding this comment.
This help string isn't quite clear. Mention multiple instances and maybe give an actual example, like --log=DEBUG --log=DCRDATA:TRACE.
There was a problem hiding this comment.
Yes, this is correct.
TRACE is not an valid option(:P) but yes, examples are totally right.
| "--log", | ||
| "-l", | ||
| action="append", | ||
| type=str, |
There was a problem hiding this comment.
You can drop the type conversion, I think. Everything is a string by default.
| args = parser.parse_args() | ||
| self.logLvls = args.log |
There was a problem hiding this comment.
Here's where you should parse the arguments, including looking for a default argument. Then prepare the root logger and any specified loggers.
|
|
||
| Return: logging.LogLvl | ||
| """ | ||
| logNameToLevel = { |
There was a problem hiding this comment.
This dict is read-only. Create it once globally.
There was a problem hiding this comment.
Done, now is before of first function definition.
| } | ||
|
|
||
| try: | ||
| lvl = int(logLvl) |
There was a problem hiding this comment.
What do you think about adding an assert logging.NOTSET <= lvl <= logging.CRITICAL here?
There was a problem hiding this comment.
My suggestion was bad, as per #47 (comment). Instead, do
if lvl > logging.CRITICAL or lvl < logging.NOTSET:
raise AssertionError("...")
| if name in logNameToLevel.keys(): | ||
| return logNameToLevel[name] | ||
| else: | ||
| return logging.NOTSET |
There was a problem hiding this comment.
return logNameToLevel[name] if name in logNameToLevel else logging.NOTSET
or maybe a better way would be
if name not in logNameToLevel:
raise AssertionError("unknown module specified: " + name)
return logNameToLevel[name]
*edited
|
@vdg0 still working on this? |
@buck54321 Yes, sorry the delay, i'm having complications with payed jobs and it's requesting more time than normal, but yes, i'm progressing with this. |
|
This is going to be a messy rebase, and there's been no progress here anyway. Closing. @vdg0: you can reopen if you get this sorted out. |
Well, i tested manually.
I put log = helpers.getLogger() before of setting it for the chance to raise Exception of bad argument on --log or -l.
I can write my firsts tests if i get some direction in how to test this...