Conversation
Bare print() to stdout for status/error/warning messages caused output buffering issues (e.g. shell command not displayed before PTY handoff) and violated the Unix convention of diagnostics on stderr. - Change default log level from WARNING to INFO so status messages remain visible; --verbose enables DEBUG; per-command --quiet raises to WARNING - Add CLIFormatter that omits level/module prefix for INFO messages, preserving the existing UX for status output - Add flush_output() utility called before subprocess/PTY handoff - Add apply_quiet() utility for commands with --quiet flag - Convert ~500 print() calls to logger.info/warning/error across 18 source files; ~265 print() calls remain for program output (tables, JSON, diffs, passthrough) - Update 96 test assertions from capsys to caplog
|
Claude finished @delano's task —— View job PR Review: Route diagnostic output through Python loggingReview Summary ✅This is a well-executed logging migration that addresses real output buffering issues while maintaining clean CLI UX. The PR successfully converts ~500 print() calls across 37 files with proper test updates. Code Quality AssessmentExcellent Implementation:
Minor Issues Found:
Security & Performance ✅
Test Coverage ✅
Recommendations
Overall Assessment: APPROVE ✅This PR significantly improves the codebase's diagnostic output handling while maintaining backward compatibility. The few minor issues are non-blocking and easily addressed in follow-up commits. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the command-line interface's output mechanism by migrating from direct Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring the CLI's output to use Python's logging module. This provides better structure, log levels, and a cleaner way to handle --quiet mode. The custom _CLIFormatter is a clever solution to maintain the existing look and feel for INFO-level messages.
I've made a few suggestions to improve consistency in logging practices and restore a test assertion that was removed. Specifically, I've pointed out a few places where f-strings are used in logger calls, which is inconsistent with the otherwise excellent use of %-style formatting. I also noticed a test assertion was removed and have suggested how to restore it using caplog.
Note: Security Review did not run due to the size of the PR.
| print(f" Size: {size:,} bytes") | ||
| logger.info("Backup created: %s", target) | ||
| logger.info(" Source: %s", db_path) | ||
| logger.info(" Size: %s bytes", f"{size:,}") |
There was a problem hiding this comment.
While this works, using an f-string for logging arguments is inconsistent with the %-style formatting used elsewhere in the codebase. This can lead to evaluating the expression even if the log level is not high enough to display the message. It would be more consistent to format the value before the logging call.
For example:
size_str = f"{size:,}"
logger.info(" Size: %s bytes", size_str)| if _path_exists(path, executor): | ||
| if not quiet: | ||
| print(f" [ok] {path}") | ||
| logger.info(f" [ok] {path}") |
There was a problem hiding this comment.
This log message uses an f-string. For consistency with the rest of the codebase and to leverage deferred formatting, please use %-style formatting.
| logger.info(f" [ok] {path}") | |
| logger.info(" [ok] %s", path) |
| logger.info(f"Initializing {pkg.name} instance '{instance}'") | ||
| logger.info(f" Template: {pkg.template_unit}") | ||
| logger.info(f" Port: {port_num}") | ||
| logger.info(f" Bind: {bind}") |
There was a problem hiding this comment.
These log messages use f-strings. For consistency and to take advantage of deferred formatting, it's better to use %-style formatting.
| logger.info(f"Initializing {pkg.name} instance '{instance}'") | |
| logger.info(f" Template: {pkg.template_unit}") | |
| logger.info(f" Port: {port_num}") | |
| logger.info(f" Bind: {bind}") | |
| logger.info("Initializing %s instance '%s'", pkg.name, instance) | |
| logger.info(" Template: %s", pkg.template_unit) | |
| logger.info(" Port: %d", port_num) | |
| logger.info(" Bind: %s", bind) |
| raise SystemExit(rc) | ||
| except KeyboardInterrupt: | ||
| print("\nStopped") | ||
| logger.info("\nStopped") |
There was a problem hiding this comment.
Including a leading newline \n in a log message is unconventional and can lead to inconsistent log formatting. It's better to log the message directly and let the logging handler manage newlines.
| logger.info("\nStopped") | |
| logger.info("Stopped") |
| assert "new_value" in captured.out | ||
| # Should indicate dry-run | ||
| assert "dry run" in captured.out.lower() or "no changes made" in captured.out.lower() | ||
|
|
There was a problem hiding this comment.
This test assertion for the dry-run message was removed. Since the output is now logged, you can restore this check using the caplog fixture to ensure the dry-run message is correctly logged.
import logging
# Inside the test function
with caplog.at_level(logging.INFO):
instance.config_transform(command="transform", dry_run=True)
captured = capsys.readouterr()
assert "old_value" in captured.out
assert "new_value" in captured.out
assert "dry run" in caplog.text.lower() or "no changes made" in caplog.text.lower()1 participant
Summary
Replaces bare
print()calls throughout the CLI with Python'sloggingmodule, giving us proper log levels, consistent formatting, and a clean mechanism for--quietmode.logger = logging.getLogger(__name__)instead ofprint()_CLIFormatterincli.pyrenders INFO as plain text (preserving existing CLI appearance) while WARNING+ gets aLEVEL module:prefixapply_quiet()helper sets root logger to WARNING, replacing scatteredif not quiet: print()guardsflush_output()helper ensures buffered log output is flushed before PTY/subprocess handoff (fixes potential output swallowing)print()calls retained for machine-readable output (JSON), raw content display, interactive prompts, and table formattingTests updated across all command modules — instance tests use a
_LiveStderrHandlerfixture (capsys on stderr), other tests usecaplog.Review notes
print()behavior)service/app.pyandinit.py— not blocking but inconsistent with%-style used elsewheretest_config_transform.pywas removed without migration to caplogTest plan
--quietsuppresses INFO output on a live system--verboseenables DEBUG output