http: warn when http.get/head receive extra arguments#5690
http: warn when http.get/head receive extra arguments#5690SydilSohan wants to merge 1 commit intografana:masterfrom
Conversation
Display a warning when http.get() or http.head() are called with more than the expected (url, params) arguments. Users commonly pass a body argument (e.g. http.get(url, body, params)) not realizing that GET and HEAD do not support request bodies. The extra arguments are silently ignored, which can lead to confusing behavior. Fixes grafana#2823 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
There was a problem hiding this comment.
Thanks for picking this up, @SydilSohan! The PR correctly addresses the problem from #2823 and has good test coverage. There are a few suggestions — mostly stemming from the unaddressed feedback on the previous attempt (#3974) referenced by this PR.
1. Extract the duplicated warning logic into a helper
The warning block is copy-pasted for get and head, differing only in the method name. This was the primary feedback from #3974 — @joanlopez explicitly suggested a validateArgCount helper, and @olegbespalov agreed. The PR description says it "incorporates the review comments from that PR," but this one wasn't addressed.
2. Use %d instead of %.0f with float64() cast
len(args)-1 is already an int. The float64() cast + %.0f format verb is unnecessary. Just use %d:
3. Consider a more actionable warning message
@olegbespalov suggested a more actionable message, and @joanlopez reiterated this. The current message is decent, but doesn't hint at what the user should do.
What?
Display a warning when
http.get()orhttp.head()are called with more than the expected(url, params)arguments.Why?
Users commonly pass a body argument (e.g.
http.get(url, body, params)) not realizing that GET and HEAD do not support request bodies. The extra arguments are silently ignored, which can lead to confusing behavior.The warning message identifies the method and how many extra arguments will be ignored:
Checklist
golangci-lint run) and all checks pass.Fixes #2823
Related: #3974 (previous attempt, closed with unaddressed feedback — this PR incorporates the review comments from that PR)