fix: replace print() with logger in responses, reloader, and __init__#1328
Open
mango766 wants to merge 4 commits intosparckles:mainfrom
Open
fix: replace print() with logger in responses, reloader, and __init__#1328mango766 wants to merge 4 commits intosparckles:mainfrom
mango766 wants to merge 4 commits intosparckles:mainfrom
Conversation
Replace print() calls with proper logging in non-CLI modules:
- responses.py: Error in async generator now uses logger.error()
instead of silently printing to stdout
- reloader.py: 6 print() calls replaced with logger.info/error,
also fixes broken format strings that used print("msg: %s", arg)
which printed the tuple literal instead of formatting
- __init__.py: Rust compilation message uses logger.info()
CLI-facing output in cli.py is intentionally left as print() since
those are user-facing messages, not log entries.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughModule-level logging was introduced and direct prints replaced with logger calls across reloader and responses; Logger now installs a NullHandler to avoid missing-handler warnings. Control flow and public APIs were unchanged. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
robyn/reloader.py (1)
15-31:⚠️ Potential issue | 🟠 MajorLogger messages will be silently dropped in dev mode.
When running
robyn --dev app.py, thesetup_reloader()function is called before anyRobyninstance is created. Sincelogging.basicConfig()is only called inBaseRobyn.__init__, the logger has no handlers configured whensetup_reloader(),compile_rust_files(), and other functions in this file execute. Python's default root logger level is WARNING, so INFO-level messages are silently dropped (logger.error() and logger.warning() calls will still appear via the lastResort handler).This affects lines 20, 31, 87, 95, and 130 where logger.info() is called.
Add logging configuration at the start of
setup_reloader():🔧 Proposed fix
def setup_reloader(directory_path: str, file_path: str) -> None: + import logging + logging.basicConfig(level=logging.INFO) + event_handler = EventHandler(file_path, directory_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/reloader.py` around lines 15 - 31, The logger is used before BaseRobyn sets up handlers, so INFO messages from setup_reloader(), compile_rust_files(), and other functions are dropped; fix by ensuring logging is configured at the start of setup_reloader() (e.g., call logging.basicConfig(...) or add a StreamHandler with level INFO if no handlers exist) so logger.info(...) calls produce output; update setup_reloader() to check logging.getLogger().handlers and configure a default handler/level (INFO) when absent so calls in compile_rust_files(), setup_reloader(), and related functions are visible in dev mode.
🧹 Nitpick comments (1)
robyn/responses.py (1)
121-124: Consider usinglogger.exception()for full traceback.The current implementation logs only the exception message. If debugging async generator issues is important,
logger.exception()would automatically include the stack trace.♻️ Optional improvement
except Exception as e: # Log error and stop iteration - logger.error("Error in async generator: %s", e) + logger.exception("Error in async generator") raise StopIteration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/responses.py` around lines 121 - 124, Replace the logger.error call in the async generator exception handler with logger.exception so the full traceback is recorded; in robyn/responses.py update the except Exception as e block that currently calls logger.error("Error in async generator: %s", e) to call logger.exception("Error in async generator") (or logger.exception with exc_info=True) immediately before raising StopIteration so the stack trace for the async generator (the exception handler around the generator function) is preserved in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@robyn/__init__.py`:
- Around line 71-73: The module-level logger.info call after compile_rust_files
(when using compile_path := config.compile_rust_path) runs at import time before
BaseRobyn.__init__ configures logging, so INFO will be dropped; either move
logging.basicConfig() to module scope before the compile block (so
logger.info(...) is handled), or replace the logger.info("Compiled rust files")
call with a direct print("Compiled rust files") (or similar CLI-friendly output)
to guarantee the compilation status is visible; locate the
compile_rust_files/compile_path check and the logger.info call in
robyn/__init__.py and update accordingly (or instead relocate
BaseRobyn.__init__'s basicConfig invocation to module level if you choose that
approach).
In `@robyn/reloader.py`:
- Around line 128-130: The "Reloading the server" message in the reload method
may be dropped by unconfigured logging; update the reload function (method
reload) to emit CLI-facing output directly (use print("Reloading the server"))
instead of logger.info, or alternatively ensure the module-level logger (logger)
has a CLI handler before calling reload (e.g., configure a
StreamHandler/basicConfig during initialization where the server is started).
Make the change in the reload method (and ensure consistency with stop_server if
it also needs CLI-visible messages).
---
Outside diff comments:
In `@robyn/reloader.py`:
- Around line 15-31: The logger is used before BaseRobyn sets up handlers, so
INFO messages from setup_reloader(), compile_rust_files(), and other functions
are dropped; fix by ensuring logging is configured at the start of
setup_reloader() (e.g., call logging.basicConfig(...) or add a StreamHandler
with level INFO if no handlers exist) so logger.info(...) calls produce output;
update setup_reloader() to check logging.getLogger().handlers and configure a
default handler/level (INFO) when absent so calls in compile_rust_files(),
setup_reloader(), and related functions are visible in dev mode.
---
Nitpick comments:
In `@robyn/responses.py`:
- Around line 121-124: Replace the logger.error call in the async generator
exception handler with logger.exception so the full traceback is recorded; in
robyn/responses.py update the except Exception as e block that currently calls
logger.error("Error in async generator: %s", e) to call logger.exception("Error
in async generator") (or logger.exception with exc_info=True) immediately before
raising StopIteration so the stack trace for the async generator (the exception
handler around the generator function) is preserved in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e4ec8bc-c119-4171-87a9-a00438155847
📒 Files selected for processing (3)
robyn/__init__.pyrobyn/reloader.pyrobyn/responses.py
Comment on lines
128
to
+130
| def reload(self) -> None: | ||
| self.stop_server() | ||
| print("Reloading the server") | ||
| logger.info("Reloading the server") |
There was a problem hiding this comment.
Reload message may not appear without handler configuration.
The "Reloading the server" message is important user feedback during hot reload. With the logging handler issue, this message will be silently dropped. Given this is CLI-facing feedback, consider keeping this as print() or ensuring logging is configured as suggested above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@robyn/reloader.py` around lines 128 - 130, The "Reloading the server" message
in the reload method may be dropped by unconfigured logging; update the reload
function (method reload) to emit CLI-facing output directly (use
print("Reloading the server")) instead of logger.info, or alternatively ensure
the module-level logger (logger) has a CLI handler before calling reload (e.g.,
configure a StreamHandler/basicConfig during initialization where the server is
started). Make the change in the reload method (and ensure consistency with
stop_server if it also needs CLI-visible messages).
Address review feedback: the compile_rust_files message at line 73 runs at module import time, before BaseRobyn.__init__ configures logging. Revert to print() for this specific CLI-facing message to ensure it is always visible.
Add logging.basicConfig(level=logging.INFO) at module level in reloader.py so that logger.info/error calls in compile_rust_files() are not silently dropped when invoked before Robyn.__init__ configures logging. Also use logger.exception() instead of logger.error() in responses.py to include traceback in async generator error handling. Co-Authored-By: Claude (claude-opus-4-6) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@robyn/reloader.py`:
- Around line 15-18: The module-level call
logging.basicConfig(level=logging.INFO) in reloader.py overrides user log
configuration; remove that basicConfig call and instead attach a NullHandler to
this module's logger (e.g.,
getLogger(__name__).addHandler(logging.NullHandler())) so early import-time
loggers are silenced until Robyn.__init__ performs full configuration, or
alternatively implement the minimal change by setting force=True in
Robyn.__init__'s logging.basicConfig(level=self.config.log_level, force=True) to
allow reconfiguration—pick one approach and apply it consistently (modify
reloader.py to add a NullHandler if you choose the first approach, or update
Robyn.__init__ if you choose the force=True approach).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ce06c6-a805-4033-8dca-d7b7667d3ecd
📒 Files selected for processing (2)
robyn/reloader.pyrobyn/responses.py
🚧 Files skipped from review as they are similar to previous changes (1)
- robyn/responses.py
…logging config Co-Authored-By: Claude (claude-opus-4-6) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
robyn/logger.py (1)
22-22: NullHandler attachment is already idempotent in practice.The
Loggerclass is instantiated only once at module level (logger = Logger()at line 81), so handler accumulation is not a concern. The current implementation at line 22 is safe and does not need modification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/logger.py` at line 22, The review points out that attaching a NullHandler is idempotent and since the Logger class is instantiated once at module level (the Logger class and the module-level instantiation logger = Logger()), no handler accumulation occurs; therefore do not change or remove the existing self.logger.addHandler(logging.NullHandler()) call in the Logger class—leave it as-is to preserve safe default handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@robyn/logger.py`:
- Line 22: The review points out that attaching a NullHandler is idempotent and
since the Logger class is instantiated once at module level (the Logger class
and the module-level instantiation logger = Logger()), no handler accumulation
occurs; therefore do not change or remove the existing
self.logger.addHandler(logging.NullHandler()) call in the Logger class—leave it
as-is to preserve safe default handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f78d81f3-4241-4d93-9d94-361e0934d438
📒 Files selected for processing (2)
robyn/logger.pyrobyn/reloader.py
🚧 Files skipped from review as they are similar to previous changes (1)
- robyn/reloader.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
print()calls with properloggerusage in non-CLI modulesreloader.pythat printed tuples instead of formatted messagesChanges
print()removedresponses.pylogger.error()reloader.pylogger.info/error__init__.pylogger.info()Bug Fix: Broken Format Strings
reloader.pyhad several broken format strings usingprint("msg: %s", arg)which doesn't do string interpolation — it just prints the tuple("msg: %s", arg). These are now fixed with properloggercalls that correctly format the arguments.Before:
After:
Note
CLI-facing output in
cli.pyis intentionally left asprint()since those are user-facing messages.Test Plan
robyn --devcorrectly shows reloader logsSummary by CodeRabbit