Skip to content
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

Jump to bottom

Proposal: net/url: add Values.Decode #32634

Closed
mvdan opened this issue Jun 15, 2019 · 7 comments
Closed

Proposal: net/url: add Values.Decode #32634

mvdan opened this issue Jun 15, 2019 · 7 comments
Labels
FrozenDueToAge Proposal Proposal-FinalCommentPeriod
Milestone
Proposal

Comments

@mvdan
Copy link
Member

mvdan commented Jun 15, 2019

Right now, if one wants to decode an URL query string into an existing non-nil Values map, the only way is something like:

values2, err := url.ParseQuery(query)
if err != nil {
    ...
}
for key, value := range values2 {
    values[key] = append(values[key], value...)
}

This means an extra allocation, and the piece of boilerplate seems like it should be unnecessary.

I think this would be much nicer, to mirror the Encode() string method we already have:

if err := values.Decode(query); err != nil {
    ...
}

The implementation would be trivial. We already have a parseQuery(m Values, query string) (err error) function, so it could be moved to the method, and ParseQuery could use the method directly.
@gopherbot gopherbot added this to the Proposal milestone Jun 15, 2019
@rsc
Copy link
Contributor

rsc commented Jun 25, 2019

Seems reasonable overall. At least two questions that need to be decided though:

  • Does Decode clear the map first?
  • If not, does it replace existing entries or does it append to them?
  • Any others?

@mvdan
Copy link
Member Author

mvdan commented Jun 26, 2019

I'd make this work similar to other encoders like encoding/json. That is, if any data is already present, the decoding inserts/appends, instead of replacing entire values.

@rsc
Copy link
Contributor

rsc commented Aug 27, 2019

/cc @bradfitz for thoughts

@andybons andybons mentioned this issue Aug 27, 2019
Open
@bradfitz
Copy link
Contributor

bradfitz commented Sep 3, 2019

The behavior on duplicate values seems kinda arbitrary. Are we sure that's what others would usually want?

And is this more for performance or for convenience?

If performance, what about the case when you only want a certain field and don't care to do the allocations for any others? The most general API might be callback-based where a user func can choose which to decode (perhaps even lazily decoding keys and/or values) and when to stop iteration.

But if it's just convenience, sure. But I might choose a more verbose name to make it clear what the overwrite-vs-append behavior is.

@mvdan
Copy link
Member Author

mvdan commented Sep 9, 2019

This isn't for performance; I didn't even benchmark or measure anything. It's convenience; I found this somewhat simple use case needed an unexpected amount of boilerplate.

Are we sure that's what others would usually want?
I might choose a more verbose name to make it clear what the overwrite-vs-append behavior is.

Hmm, if we're not sure about this, then it's probably best to reject this proposal. I'd prefer the user having to write an extra five lines, over us having a new bit of API that needs careful documentation so that users know what it does. The user's five extra lines would already obviously show what the code is meant to do.

@rsc
Copy link
Contributor

rsc commented Sep 12, 2019

The previous comment by @mvdan (the original author) suggests declining this proposal.
Likely decline; leaving open for a week for final comments.

@rsc
Copy link
Contributor

rsc commented Sep 18, 2019

No comments, so proposal declined.

@rsc rsc closed this as completed Sep 18, 2019
@golang golang locked and limited conversation to collaborators Sep 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-FinalCommentPeriod
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@bradfitz @rsc @mvdan @gopherbot