Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the custom log formatter _Formatter by removing a reimplementation of logging.Formatter.format. The new implementation overrides formatMessage to apply decolorization, and relies on the superclass's format method for the main formatting logic. This change reduces code duplication. A compatibility shim for typing.override has been added to support older Python versions. The changes are correct.
thrix
left a comment
There was a problem hiding this comment.
Clean simplification. The old format() duplicated CPython's Formatter.format() logic; the new formatMessage() override applies decolorization at the right point while inheriting all the standard exc_info/stack_info handling. The hasattr(record, 'message') check was effectively a no-op since getMessage() sets it anyway. Nice -35 lines.
Signed-off-by: Cristian Le <git@lecris.dev>
Comparing with the current implementation, the only difference we make is injecting `_decolorize` which could equally be added to `formatMessage` Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
1b4349d to
858e773
Compare
|
Marked as blocked by #4676 (the first commit in the series), otherwise should still be fine for review |
For reference the original code for Python3.9 that we still support is:
https://github.com/python/cpython/blob/0bbaf5de9744ae1acea3e2c9ad2257d1cc68e847/Lib/logging/__init__.py#L650-L680
other than the check for
record.message(which effectively makes no difference if it is re-rendered withrecord.getMessage()) I do not see any other functional change here.Pull Request Checklist