WIP: Handle Logger.exception() outside "except" block#635
WIP: Handle Logger.exception() outside "except" block#635sscherfke wants to merge 3 commits intohynek:mainfrom
Conversation
src/structlog/processors.py
Outdated
| if exc_info: | ||
| if exc_info != (None, None, None): |
There was a problem hiding this comment.
looks like this was the actual bug, right? because a negative result from _figure_out_exc_info isn't falsey
There was a problem hiding this comment.
Yes, more or less. The (None, None, None) case was handled elsewhere (surprisingly, from a typing perspective ;-)), but it only worked for the default exception formatter.
src/structlog/processors.py
Outdated
| _figure_out_exc_info(exc_info) | ||
| ) | ||
| exc_info = _figure_out_exc_info(event_dict.pop("exc_info", None)) | ||
| if exc_info != (None, None, None): |
There was a problem hiding this comment.
It seems like this would pass any None/Falsy exc_info in the event dict into the formatter. Is this expected?
There was a problem hiding this comment.
Changed it to if exc_info and exc_info != (None, None, None). This should work.
Not happy though, that exc_info can be literally anything. I will take a look if this can somehow be changed/improved.
| assert {"exception": "MISSING"} == format_exc_info( | ||
| None, None, {"exc_info": ei} | ||
| ) | ||
| assert {} == format_exc_info(None, None, {"exc_info": ei}) |
There was a problem hiding this comment.
Is this okay for you, @hynek ? The format_exc_info() handled the (None, None, None) (even though its signature suggested that exc_info: ExcInfo) and returned {"excepiton": "MISSING"}.
Now, (None, None, None) will no longer be passed to it and thus there will no longer be a MISSING.
The main problem of the function is that *v* can really be anything (depending on what the users) do and the annotated return type did not properly match it (e.g., if *v* was "0", the return value would be "0"). The function now rigorously checks the user input and either returns the desired result (an exc info tuple) or None. This makes it easier and safer to use this function.
|
I think that 0c02f29 really fixes the problem in the right way. I added a longer description to the commit to explain why. Let me know, what you think :) |
|
close on the wish of the author due to lack of feedback from OP |
|
@hynek could you please re-open this MR? I think we misunderstood each other. The issue is still relevant and the MR went in the right direction. I will finish it so that it can be mergd. |
|
looks like I can't reopen it – you'll have to recreate a new one. never seen this before |
Fixes: #634
Summary
Pull Request Check List
mainbranch – use a separate branch!api.py.docs/api.rstby hand.versionadded,versionchanged, ordeprecateddirectives..rstand.mdfiles is written using semantic newlines.