mmaney
commented
4 years ago For those not digging into the pull request message(s) - I was wrong in the original bug in that passing the one true logger in to the client (and then on to the dns driver) isn't IMO so great. It would work, but logging already manages nested instances just fine IF you check correctly before installing a local handler. That's what the pull requests are about.
Also note that #125 almost fixed this already, but the test before installing a local handler was wrong (was already wrong?).
Oh, and there's a third incorrect check (same boilerplate, same fix could be made) in cli.py, but I'm not sure the check matters when you're creating the root logger.
Py3.7, sewer 0.7.9, Debian 10. Run cli, see every log message twice. Once each is enough!
(this is actually a fairly old annoyance, just hasn't been annoying enough before now.)
This got really annoying while I was trying to get the changes made to make sewer work with the rfc8555 version of the protocol which staging now requires (and production will require before the end of this year). I hacked it by removing all the handler setup in client.py, and that was probably left in the patch for rfc8555 that I attached in #128 (should have been #135).
That fixed my immediate eye pain, and is actually the right way to do logging from a library, I think. No local handler, so log messages look for an upstream logger to actually handle them. Running the cli, that upstream is provided. It might change the behavior when client.py is used from some other driving script, though such scripts would seem likely to wish to setup their own logger anyway. If changing the API is acceptable, I'd think the logger ought to be passed in (and could be created as it has been if it's not passed?).