feat: use already calculated exc_text #48
Conversation
spaceone
force-pushed
the
fix/record-exc-text
branch
from
2495e76 to
5975a5a
Compare
src/logfmter/formatter.py
Outdated
| value = "".join(traceback.format_exception(*exc_info)).rstrip("\n") | ||
|
|
||
| return cls.format_string(value) | ||
| return "".join(traceback.format_exception(*exc_info)) |
There was a problem hiding this comment.
This change makes it so the return value of format_exc_info is no longer formatted with cls.format_string. Why is this necessary? I would rather not have a breaking change if its not necessary.
There was a problem hiding this comment.
it's also possible to use the upstream formatException() method.
I will rewrite it to be backwards compatible.
There was a problem hiding this comment.
hm, but exc_text shouldn't contain the escaped value.
So, I see only the possibility to leave the method in there, but unused.
spaceone
force-pushed
the
fix/record-exc-text
branch
from
5975a5a to
349513a
Compare
|
@spaceone let's just remove that format_exc_info function now that it isn't necessary, nice work! |
|
okay, i'll do it. |
The logging module sets the traceback string to `record.exc_text` and doesn't caluclate it again. This allows also to log exceptions, when we are not in the stack-context anymore and have no `exc_info` tuple. For example, in some cases, only the textual representation of a traceback is stored because the traceback object would be a memory leak and prevent garbage collection. Use case is therefor: `formatter.format(record)` where `exc_text` was already created by another earlier formatter or filter.
spaceone
force-pushed
the
fix/record-exc-text
branch
from
349513a to
2ff70ad
Compare
|
done |
|
@josheppinette why do you have the conventional commit message guideline, when you always squash the commits when merging? I have explicitly in multiple merge requests split the commits into logical units, in 8f3dbc1 even explaining the reasoning, which is now lost after merging it in a squashed way. |
|
My view is that a pull request should represent a single unit of change. During the development of a pull request, multiple commits may be used to generate that single unit of change. If it feels like there should be multiple commits in the main history, then it probably means the commit should be split up. However, I think all of your changes were properly sized to a single commit. Regarding the explanation, that is my bad. I merge pull requests all day that are usually full of random stuff in there that I delete and just did the same for yours. I will make sure to review more thoroughly and properly retain that information. Great job on writing those by the way. |
2 participants
The logging module sets the traceback string to
record.exc_textand doesn't caluclate it again.This allows also to log exceptions, when we are not in the stack-context anymore and have no
exc_infotuple.For example, in some cases, only the textual representation of a traceback is stored because the traceback object would be a memory leak and prevent garbage collection.
Use case is therefor:
formatter.format(record)whereexc_textwas already created by another earlier formatter or filter.