Conversation
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - This is solid engineering.
What this does right:
- Solves a real, measurable problem (40+ instances of error-prone boilerplate)
- Simple solution: 3-line function with optional parameter
- Backward compatible: zero breaking changes
- Applied consistently across the entire codebase
- Eliminates special case handling at all call sites
The "Linus Test":
- Real problem? ✅ Yes - verbose, repetitive markup pattern that is error-prone
- Simpler way? ✅ This IS the simple way - optional wrapper parameter
- What breaks? ✅ Nothing - backward compatible design
Verdict: Clean refactoring that improves maintainability without adding complexity. The abstraction level is appropriate and the implementation is straightforward. This is the kind of pragmatic cleanup that makes codebases better.
Ship it. 🚀
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - This is solid engineering.
What this does right:
- Solves a real, measurable problem (40+ instances of error-prone boilerplate)
- Simple solution: 3-line function with optional parameter
- Backward compatible: zero breaking changes
- Applied consistently across the entire codebase
- Eliminates special case handling at all call sites
The "Linus Test":
- Real problem? ✅ Yes - verbose, repetitive markup pattern that is error-prone
- Simpler way? ✅ This IS the simplest way - optional param wraps the message once
- What breaks? ✅ Nothing - backward compatible, tests confirm this
Key Insight:
Moving the markup wrapping from 40+ call sites into one function is textbook "good taste" - the new call sites have no special cases, just console_print("msg", style=THEME.success). Clean separation of message from styling.
Previous review comments about incomplete refactoring and missing tests have been addressed. The code is ready.
✅ Approved
What: Extended console_print in
auth/utils.pywith an optional style keyword parameter that auto-wraps the message in[{style}]...[/{style}] markup. Updated ~40 call sites across 6 files:
Why: The verbose f"[{OPENHANDS_THEME.X}]msg[/{OPENHANDS_THEME.X}]" pattern was repeated 40+ times — error-prone (easy to mismatch open/close tags) and noisy. Now callers just write console_print("msg", style=OPENHANDS_THEME.success).
The style parameter is backward-compatible so zero test changes were needed.
Note:
When I refactored cloud/command.py, I removed the local console = Console() instance and replaced all console.print(...) calls with console_print(...) (imported from auth/utils.py).
But the two tests in test_main.py were patching openhands_cli.cloud.command.console — an attribute that no longer existed in the module. So patch() raised AttributeError.
The fix was simply updating those two test mocks to patch openhands_cli.cloud.command.console_print instead, matching the new function the module actually uses.
🚀 Try this PR