fix: write errors/warnings to stderr rather than stdout #651
Conversation
WalkthroughThe changes in this pull request focus on enhancing error logging across multiple files: Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #651 +/- ##
==========================================
+ Coverage 92.97% 93.04% +0.06%
==========================================
Files 22 22
Lines 4895 4901 +6
==========================================
+ Hits 4551 4560 +9
+ Misses 300 298 -2
+ Partials 44 43 -1 ☔ View full report in Codecov by Sentry. |
sm3142
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
error.go(2 hunks)transforms.go(2 hunks)
🔇 Additional comments (4)
transforms.go (2)
6-6: LGTM: Import added correctly
The os package import is correctly added and properly ordered within the import block.
136-136: LGTM: Error output correctly redirected to stderr
The change properly implements the PR objective by redirecting the warning message to stderr. This is particularly beneficial when redirecting stdout to files (e.g., go run . openapi > openapi.yaml), as warning messages will no longer interfere with the generated output.
error.go (2)
7-7: LGTM: Required import for stderr functionality
The addition of the "os" package import is necessary and properly placed.
268-268: LGTM: Appropriate use of stderr for error logging
The change from fmt.Printf to fmt.Fprintf(os.Stderr, ...) aligns with Unix convention and best practices where error messages should be written to stderr. This fixes the issue where error messages were interfering with stdout operations like redirecting output to files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sse/sse.go (2)
164-167: Consider enhancing the warning message for better debugging.The warnings are now correctly directed to stderr. However, the warning message on line 167 could be more informative.
Consider this enhancement:
-fmt.Fprintln(os.Stderr, "warning: unable to set write deadline") +fmt.Fprintln(os.Stderr, "warning: write deadline not supported by the underlying writer")
Line range hint
180-182: Missed stdout usage that should be converted to stderr.This error message is still using stdout (fmt.Println) while all other errors have been converted to use stderr.
Apply this change for consistency:
-fmt.Println("error: unknown event type", reflect.TypeOf(msg.Data)) +fmt.Fprintf(os.Stderr, "error: unknown event type %v\n", reflect.TypeOf(msg.Data))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
humacli/humacli.go(1 hunks)sse/sse.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- humacli/humacli.go
🔇 Additional comments (2)
sse/sse.go (2)
9-9: LGTM: Import added for stderr redirection.
The addition of the "os" import is necessary and correctly placed.
202-204: LGTM: Error handling improved with stderr.
The error is now correctly written to stderr and properly wrapped with http.ErrNotSupported.
What
Tiny PR to send Huma's (rare) direct error/warning output to
stderrrather thanstdoutWhy
stderrgo run . openapi > openapi.yamland the warnings fromtransforms.go:135were mucking up the Open API spec.How
Code should be obvious enough without further exposition
[1] I realize I still have to deal with the actual cause for the warnings in my code, but they are incidentally also more visible when sent to
stderr.Testing
AFAICT, the statements are not currently covered by test cases. IMHO the changes are too trivial to justify the cost of setting up explicit tests.
Thanks for considering this PR!
Summary by CodeRabbit
Bug Fixes
New Features