Skip to content

Comments

Ensure ColorFormatter restores original levelname for sibling handlers#2309

Open
KristopherKubicki wants to merge 1 commit intostagingfrom
codex/adjust-colorformatter-to-cache-levelname
Open

Ensure ColorFormatter restores original levelname for sibling handlers#2309
KristopherKubicki wants to merge 1 commit intostagingfrom
codex/adjust-colorformatter-to-cache-levelname

Conversation

@KristopherKubicki
Copy link
Owner

Summary

  • update ColorFormatter to temporarily colorize the level name and restore the original value after formatting
  • add a regression test that ensures a plain handler stays uncolored when paired with a colored handler on the same logger

Testing

  • pytest tests/test_color_formatter.py

Codex Task

Copilot AI review requested due to automatic review settings November 14, 2025 04:26
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/adjust-colorformatter-to-cache-levelname

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +46 to +50
colored_handler.close()
plain_handler.close()
logger.propagate = original_propagate

colored_output = colored_stream.getvalue().strip()

Choose a reason for hiding this comment

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

P1 Badge Read StringIO after closing underlying stream

The regression test closes both handlers, but StreamHandler.close() also closes the attached StringIO. When execution reaches colored_stream.getvalue().strip() the stream is already closed and Python raises ValueError: I/O operation on closed file, so the test never reaches its assertions. Fetch the buffered contents before calling close() (or avoid closing the handlers) so the test exercises the behavior it is meant to verify.

Useful? React with 👍 / 👎.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where ColorFormatter would permanently modify the levelname attribute of LogRecord objects, causing sibling handlers (that share the same LogRecord instance) to incorrectly receive colorized level names even when using plain formatters.

Key Changes:

  • Modified ColorFormatter.format() to save, modify, and restore the original levelname using a try/finally block
  • Added a regression test verifying that plain handlers remain unaffected when paired with colored handlers on the same logger
  • Added a conditional check to only modify levelname when a color mapping exists

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
app/utils/logging_utils.py Implements levelname preservation in ColorFormatter using try/finally to restore the original value after formatting
tests/test_color_formatter.py Adds regression test verifying plain handlers stay uncolored when used alongside colored handlers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions
Copy link

Stale pull request message

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant