correcting logic to discern the lowest logging level

[todd-cook]

re: #70

I extracted the method and created a doctest using the example config (from https://github.com/kquick/Thespian/blob/master/examples/logsetup.py#L10).

Tested with Docker and Kubernetes

[kquick]

Thank you! I have a few comments on the PR, but I wanted to say that I appreciate the submission.

[kquick]

Great enhancements, but unfortunately a couple of problems that need to be fixed. Thanks for your continued effort on this!

[kquick]

It seems to me that this should exclude all NOTSET values, irrespective of root_val, since allowing any NOTSET would result in a return value of 0 if there was no root_val specified.

Also, the code for setting root_val does not ensure that root_val is numeric: it could be None, or it could be an unrecognized string like "MyLevel", which would cause the > comparison to throw a TypeError exception.

If there's no comparison to root_val here, then the reduction would lead to removing root_val as a variable and just putting the result of the corresponding expression in the levels array.

Let me know if you disagree with my analysis here or if there's something I've missed.

[todd-cook]

NOTSET is useful because the default logging level is warning. If somebody writes some library code and configs the logs somewhere and specifies NOTSET, it does log everything, but if a root logger specifies a level then it supercedes the NOTSET value. But if a logging config had all NOTSET values, and we discarded them, then the configured logging level would default to WARNING, which seems to violate the principle of least surprise.

Also, it looks like root value can be none:

logDefs = {'version': 1, 'root': {'level': 10}, 'loggers': {'one': {'level': 20}}}
logDefs2 = {'version': 1, 'root': None, 'loggers': {'one': {'level': 20}}}
logging.config.dictConfig(logDefs)
logging.config.dictConfig(logDefs2)

I like this validation, so I added it to the doctests.

[todd-cook]

I've adjusted the logic. Invalid strings blow up the underlying Python API, so I think that error is fair game.

[kquick]

I like those two logDefs as well, although I don't see that you added them to the doctests.

I agree on the unrecognized strings issue, it's reasonable to have similar behavior.

I'm not seeing behavior where NOTSET logs everything. See https://gist.github.com/kquick/d554a8b4de5541eb48c0bc1b14080fff which is careful to run a separate python for each configuration. I believe the last output set is the case you indicated should log everything but I see only WARNING and ERROR logging from that output (shown in this gist: https://gist.github.com/kquick/98eac6e6c55fa14fe3fbb19d671ed4b2)

These are the two cases that will still fail:

>>> logDefs = {'version': 1,
...             'root': {'level': None},
...            'loggers': {'one': {'level': logging.NOTSET}}}
>>> logging.config.dictConfig(logDefs)
>>> get_min_log_level(logDefs) == logging.WARNING
True

>>> logDefs = {'version': 1,
...            'loggers': {'one': {'level': logging.NOTSET}}}
>>> logging.config.dictConfig(logDefs)
>>> get_min_log_level(logDefs) == logging.WARNING
True

[todd-cook]

NOTSET on the root level logs everything, but NOTSET on another level besides root, logs at the root level, and if unintialized the logging level goes to WARNING.
I added a 6th test case to your gist that I co-opted.
https://gist.github.com/todd-cook/883662ba19eb5a3bb8b372733ec7f6a4

[todd-cook]

see: https://gist.github.com/todd-cook/883662ba19eb5a3bb8b372733ec7f6a4#file-testlog-py-L43-L61
and output at:
https://gist.github.com/todd-cook/883662ba19eb5a3bb8b372733ec7f6a4#file-testlog-py-L114-L135

[kquick]

So many different combinations and effects! The latest get_min_log_level in this PR will return the wrong value for case 4 and case 5 if you add those to the doctests.

Also, you can change the doctests from:

>>> get_min_log_level(logDevs) == logging.WARNING
True

to the following which will provide more information for when the tests fail:

>>> get_min_log_level(logDefs)
30

[todd-cook]

Thanks for your feedback and patience!

[kquick]

Thank you for the contribution and working with me to resolve all the corner cases. It's very much appreciated!

[todd-cook]

Kevin, for further PRs, this is the right repo and fork approach that you prefer?
https://thespianpy.com/doc/#outline-container-orgd95dc23 Contributions section points to this/your repo.
But the pypi and travis stats seem to be out of: https://github.com/thespianpy/Thespian and the PRs there are old.
I like the history of the project and preserving things. Just didn't want to be annoying you with PRs to your fork if there's a more direct way.

[kquick]

Either repo is fine, and I monitor both and keep them generally in sync.

The original was published by my previous employer for whom I did most of the initial development. After I left, I maintained my fork actively whereas they did no further updates. After a number of years, they transferred the original to me under the thespianpy group, but I didn't want to break any links or other references to my kquick version that folks might be maintaining, so I've just kept both in parallel.

Thanks for checking in though: I should clarify this situation in the README and ensure the documentation and CI is updated.