Skip to content

Simplify the log._Formatter#4654

Open
LecrisUT wants to merge 3 commits intoteemtee:mainfrom
LecrisUT:chore/use-inheritance
Open

Simplify the log._Formatter#4654
LecrisUT wants to merge 3 commits intoteemtee:mainfrom
LecrisUT:chore/use-inheritance

Conversation

@LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Mar 6, 2026

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

        record.message = record.getMessage()
        if self.usesTime():
            record.asctime = self.formatTime(record, self.datefmt)
        s = self.formatMessage(record)
        if record.exc_info:
            # Cache the traceback text to avoid converting it multiple times
            # (it's constant anyway)
            if not record.exc_text:
                record.exc_text = self.formatException(record.exc_info)
        if record.exc_text:
            if s[-1:] != "\n":
                s = s + "\n"
            s = s + record.exc_text
        if record.stack_info:
            if s[-1:] != "\n":
                s = s + "\n"
            s = s + self.formatStack(record.stack_info)
        return s

other than the check for record.message (which effectively makes no difference if it is re-rendered with record.getMessage()) I do not see any other functional change here.

Pull Request Checklist

  • implement the feature

@LecrisUT LecrisUT added code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. ci | full test Pull request is ready for the full test execution review | trivial Very easy for review, even for beginners, so don't be afraid to have a look! :-) labels Mar 6, 2026
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thrix thrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@happz happz added this to planning Mar 11, 2026
@github-project-automation github-project-automation bot moved this to backlog in planning Mar 11, 2026
@happz happz moved this from backlog to review in planning Mar 11, 2026
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT LecrisUT added the status | blocked The merging of PR is blocked on some other issue label Mar 11, 2026
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>
@LecrisUT LecrisUT force-pushed the chore/use-inheritance branch from 1b4349d to 858e773 Compare March 11, 2026 14:54
@LecrisUT
Copy link
Contributor Author

Marked as blocked by #4676 (the first commit in the series), otherwise should still be fine for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci | full test Pull request is ready for the full test execution code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. review | trivial Very easy for review, even for beginners, so don't be afraid to have a look! :-) status | blocked The merging of PR is blocked on some other issue

Projects

Status: review

Development

Successfully merging this pull request may close these issues.

3 participants