-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a logging.Filter implementation #52
base: main
Are you sure you want to change the base?
Add a logging.Filter implementation #52
Conversation
Note: requires #51 |
Note: I am capable and willing to maintain this in a separate repo / package if you think that the feature is not in scope for this project. Just let me know... |
Hey @znerol Thank you very much for this contribution 🚀! I'm sorry you had to ping me. I like this feature a lot and I think it absolutely is in the scope of this project. I'm very absorbed by work and life right now, but I see some light at the end of the tunnel by the end of next week. Maybe I already find some time this weekend, but I really can't promise. |
I guess I have to update the docs anyway (params changed since the first commit). Also it might be helpful to provide a working example for an existing app. I'm planning to use this in matrix-org/synapse thus will try to come up with a working config for that project. |
3e8ebb1
to
fe9f794
Compare
Testing this with the following
Regrettably |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks a lot 🚀
I'd like to play around with it a bit, as soon as the tests pass.
""" | ||
See logging.Filter.filter() | ||
""" | ||
if record.name != "anonip": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to make this method more readable. As a start we could exit early with
if record.name == "anonip":
return True
Just as a heads up: I just renamed the |
Hey @znerol I had some time to play around with it and made a suggestion in #59. There I have squashed your 12 commits into one, rebased onto current With my commit things seem to work and tests are passing (coverage is still lacking tho). I did mostly look and fix the I have the impression, that the logging module puts the args into a tuple, if you provide just one string. That's why I wrote the test accordingly. But maybe I'm mistaken. You could now cherry-pick my commit into your branch and squash all commits together and use it as a base to finish this feature (I don't care if you will be the sole author then ;)). On another note: I'm planning on releasing v1.1.0 in the coming days. Would be nice to land this feature in Happy holidays to you! |
There is no need to rush this into 1.1.0. I still dislike the sheer number of control statements / conditionals in both implementations. I'd like to try breaking the whole thing up into multiple filter classes. One for The filter needs to be attached to a specific logger instance anyway. According to my tests with |
See #60 |
This PR adds an
AnonipFilter
which can be used as a filter in the Pythonlogging
framework. The filter can either be hard-coded when initializing thelogging
framework usinglogging.basicConfig
, or it can be supplied in thefilters
entry oflogging.dictConfig
. A working example of the latter approach: