-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark some of the option fields as ignored in pkg/bindings #15491
Mark some of the option fields as ignored in pkg/bindings #15491
Conversation
I realized that `params.Del("SkipTLSVerify")` doesn't have any effect because keys are always lowercased. So it should really be `params.Del("skiptlsverify")`. There's also a little bug introduced by 3bf52aa and b1d1248: if one passes `ProgressWriter` object having `Stringer` interface i.e. `bytes.Buffer` it ends up been serialized in query with `util.ToParams()`. To circumvent both problems I propose to mark non-serializable parameters with `schema:"-"` so there's no need to delete them from resulting `url.Values`. Signed-off-by: Vladimir Kochnev <[email protected]>
28961bb
to
cfdca82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwhonce can you take a look?
LGTM |
Bump |
@baude PTAL |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, marshall-lee The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
I realized that
params.Del("SkipTLSVerify")
doesn't have any effect because keys are always lowercased. So it should really beparams.Del("skiptlsverify")
.There's also a little bug introduced by 3bf52aa and b1d1248: if one passes
ProgressWriter
object havingStringer
interface i.e.bytes.Buffer
it ends up been serialized in query withutil.ToParams()
.To circumvent both problems I propose to mark non-serializable parameters with
schema:"-"
so there's no need to delete them from resultingurl.Values
.Does this PR introduce a user-facing change?