Skip to content

Conversation

KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Aug 26, 2025

User description

small improvement imo, thoughts?


PR Type

Enhancement


Description

  • Introduce RuleAfterLogHandler for log separators

  • Replace RichHandler with RuleAfterLogHandler in config

  • Update verbose logging format "function" → "fn"

  • Remove final console.rule() call


File Walkthrough

Relevant files
Enhancement
logging_config.py
Use custom handler for log separators                                       

codeflash/cli_cmds/logging_config.py

  • Added RuleAfterLogHandler subclass overriding emit to call
    console.rule()
  • Switched basicConfig handlers to use RuleAfterLogHandler
  • Removed standalone console.rule() at end of set_level
  • Updated VERBOSE_LOGGING_FORMAT wording from "function" to "fn"
+12/-4   

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Handler Definition

RuleAfterLogHandler is defined inside set_level on each invocation, consider moving it to module level to avoid repeated class creation and improve readability.

class RuleAfterLogHandler(RichHandler):
    def emit(self, record: logging.LogRecord) -> None:
        super().emit(record)
        console.rule()
basicConfig Usage

Multiple calls to logging.basicConfig without force=True in the initial configuration may lead to handler duplication or unexpected behavior; consider consolidating configuration or using force=True consistently.

logging.basicConfig(
    level=level,
    handlers=[
        RuleAfterLogHandler(rich_tracebacks=True, markup=False, console=console, show_path=False, show_time=False)
    ],
    format=BARE_LOGGING_FORMAT,

@KRRT7
Copy link
Contributor Author

KRRT7 commented Aug 26, 2025

new
Screenshot 2025-08-26 at 5 49 39 AM

old
Screenshot 2025-08-26 at 5 50 21 AM

@KRRT7 KRRT7 requested a review from a team August 26, 2025 10:50
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Enforce basicConfig override

Add force=True to the initial basicConfig call so that your new handler and format
are applied even if logging was configured earlier in the application.

codeflash/cli_cmds/logging_config.py [19-25]

 logging.basicConfig(
     level=level,
     handlers=[
         RuleAfterLogHandler(rich_tracebacks=True, markup=False, console=console, show_path=False, show_time=False)
     ],
     format=BARE_LOGGING_FORMAT,
+    force=True,
 )
Suggestion importance[1-10]: 7

__

Why: Adding force=True ensures the initial basicConfig applies the new handler and format even if logging was previously configured, improving reliability.

Medium
General
Extract handler class to module level

Move the custom handler class to the module level to avoid redefining it on every
invocation of set_level. This reduces runtime overhead and simplifies testing.

codeflash/cli_cmds/logging_config.py [14-17]

+from rich.logging import RichHandler
+
 class RuleAfterLogHandler(RichHandler):
     def emit(self, record: logging.LogRecord) -> None:
         super().emit(record)
         console.rule()
 
+def set_level(level: int, *, echo_setting: bool = True) -> None:
+    import logging
+    import time
+    # ... rest of set_level unchanged ...
+
Suggestion importance[1-10]: 6

__

Why: Moving RuleAfterLogHandler outside set_level avoids redefining the class on each call, reducing runtime overhead and improving testability.

Low
Guard console rule call

Wrap the console.rule() call in a try/except block to prevent any rendering errors
from interrupting log emission, ensuring the handler remains resilient.

codeflash/cli_cmds/logging_config.py [15-17]

 def emit(self, record: logging.LogRecord) -> None:
     super().emit(record)
-    console.rule()
+    try:
+        console.rule()
+    except Exception:
+        pass
Suggestion importance[1-10]: 5

__

Why: Wrapping console.rule() in a try/except prevents rendering errors from disrupting log emission, adding resilience with minimal overhead.

Low

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