Skip to content

Simplify the log._Formatter#4654

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

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

Conversation

@LecrisUT
Copy link
Member

@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
@LecrisUT LecrisUT mentioned this pull request Mar 11, 2026
@LecrisUT LecrisUT added the status | blocked The merging of PR is blocked on some other issue label Mar 11, 2026
@LecrisUT LecrisUT force-pushed the chore/use-inheritance branch from 1b4349d to 858e773 Compare March 11, 2026 14:54
@LecrisUT

This comment was marked as resolved.

LecrisUT added a commit that referenced this pull request Mar 16, 2026
Separated from #4654. See there for more context of why it is needed

Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT LecrisUT removed the status | blocked The merging of PR is blocked on some other issue label Mar 16, 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 858e773 to b9a9222 Compare March 16, 2026 14:27
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! :-)

Projects

Status: review

Development

Successfully merging this pull request may close these issues.

3 participants