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

proposal: net/url: add (Values).AddAll(key string, values []string) #33498

Closed
tomocy opened this issue Aug 6, 2019 · 6 comments
Closed

proposal: net/url: add (Values).AddAll(key string, values []string) #33498

tomocy opened this issue Aug 6, 2019 · 6 comments

Comments

@tomocy
Copy link
Contributor

tomocy commented Aug 6, 2019

The current (Values).Add, you know, enables us to add one value to one key, but is less convenient sometimes, especially when you want to add multiple values to one key (etc: coping one Values to another Values).

func copyValues(dest, src url.Value) {
    for k, vs := range src {
        for _, v := range vs {
            dest.Add(k, v)
        }
    }
}

If (Values).AddAll is available, you can write like the blow.

func copyValues(dest, src url.Value) {
    for k, vs := range src {
        dest.AddAll(k, vs)
    }
}

It may make code clearer and improve it. (The code seems to be O(N).)

Is it good idea to (Values).AddAll to add multiple values to one key at the same time?

@gopherbot gopherbot added this to the Proposal milestone Aug 6, 2019
@OneOfOne
Copy link
Contributor

OneOfOne commented Aug 6, 2019

vals[key] = append(vals[key], newkeys...)?

@Freeaqingme
Copy link

Could you perhaps elaborate how much this would improve a real-world application?

@rsc rsc changed the title Proposal: net/url: add (Values).AddAll(key string, values []string) proposal: net/url: add (Values).AddAll(key string, values []string) Aug 6, 2019
@rsc
Copy link
Contributor

rsc commented Aug 6, 2019

This is Add:

func (v Values) Add(key, value string) {
	v[key] = append(v[key], value)
}

It's only a convenience function; it would be fine to write the append.

It is also fine to write @OneOfOne's version when you have multiple values.
It would make sense to have a convenience function if this happened a lot but does it (@Freeaqingme's question)?

The complexity analysis in the top comment is not accurate. Assuming AddAll uses @OneOfOne's append, the loops are identical in runtime - both are O(N), because append is amortized O(1).

@tomocy
Copy link
Contributor Author

tomocy commented Aug 19, 2019

I just found two cases of this in golang/go

@rsc
Copy link
Contributor

rsc commented Aug 20, 2019

Thanks @tomocy. I filed #33744 for the internal helper in net/http.
Internal helpers, of course, don't warrant external API.

Given the lack of evidence that this happens a lot, this seems like a likely decline.
Will leave open for a week for final comment period.

@rsc
Copy link
Contributor

rsc commented Aug 27, 2019

Marked this last week as likely decline w/ call for last comments (#33498 (comment)).
Declining now.

@rsc rsc closed this as completed Aug 27, 2019
@golang golang locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants