-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
#19 Pass positional arguments down to stdlib #23
Conversation
poke :) |
Thanks! Could you add docstrings to the tests in the style of https://plus.google.com/+JonathanLange/posts/YA3ThKWhSAj ? I know the original tests of structlog didn’t do that but that was also my way to enlightenment why one should do it. :) |
@@ -183,6 +184,7 @@ def _proxy_to_logger(self, method_name, event=None, **event_kw): | |||
""" | |||
try: | |||
args, kw = self._process_event(method_name, event, event_kw) | |||
args = args[:] + event_args |
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.
Any reason why you copy args? Why not just args += event_args
?
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.
I’m also a bit confused. There’s the addition of an formatter for stdlib strings but still the args won’t get passed into the processor chain but passed into the wrapped method instead? Am I missing something?
ISTM that there’s also no test for this behavior. :)
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.
This is actually old code, was needed my previous implementation and I forgot to remove it.
Would you mind helping out? :) @gcbirzan seems rather busy and my own stdlib logging expertise is rather limited. :( |
@hynek I can give it a shot, yes. |
Amazing! |
b45b077
to
ba6593e
Compare
Closed via #34. |
Apparently, there's an error checking merge status :)