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

change the name of merge or at least mention differences? #10

Open
Roger-luo opened this issue Nov 10, 2020 · 7 comments
Open

change the name of merge or at least mention differences? #10

Roger-luo opened this issue Nov 10, 2020 · 7 comments

Comments

@Roger-luo
Copy link
Contributor

Roger-luo commented Nov 10, 2020

I was trying to merge some new query params to existing ones with merge, but to my surprise that merge creates a new URI with path, query etc. instead of merging them with the old ones. This is quite confusing to me before I read the implementation since the merge on Dict actually "merge" the old one with the new one together. I'm wondering if the name of merge should be changed to something else so it is clearer?

On the other hand, I'm wondering if there could be such a method that adds some new properties to the existing query. E.g it's useful when a REST API is provided via a path https:://auth.ibmq.com/api which is parsed as "https:://auth.ibmq.com" and "/api" as its path, then a method that join the path of the REST API calls would be very convenient to produce things like "https:://auth.ibmq.com/api/loginWithAuth"

PS. I think probably let the URI constructor take some keywords makes more sense than overloading merge in this case.

@c42f
Copy link
Collaborator

c42f commented Nov 11, 2020

I'm wondering if the name of merge should be changed to something else so it is clearer?

Note that generic URIs syntax has no concept of key value pairs for the query portion - it simply has a string for the query. This is why URIs.queryparams is a utility function for the common convention of &-separated key value pairs rather than these being parsed as part of URI construction. Key-value pairs are mentioned in https://tools.ietf.org/html/rfc3986#section-3.4 but it's only an example, not meant to be normative.

So I'm not quite sure we should change merge to manipulate the query string. Though I can see that it makes a certain amount of sense.

then a method that join the path of the REST API calls would be very convenient

I think joinpath(::URI, relpath::AbstractString) would have the correct semantics? How about this sketch

julia> u = URI("http://example.com/foo")
URI("http://example.com/foo")

julia> Base.joinpath(u::URI, path_components::AbstractString...) = merge(u, path=join((u.path, path_components...), '/'))

julia> joinpath(u, "bar")
URI("http://example.com/foo/bar")

julia> joinpath(u, "bar", "baz")
URI("http://example.com/foo/bar/baz")

PS. I think probably let the URI constructor take some keywords makes more sense than overloading merge in this case.

Yes, I think this should be done and we should deprecate the current merge which has a specific meaning not really consistent with Base.merge.

@Roger-luo
Copy link
Contributor Author

Note that generic URIs syntax has no concept of key value pairs for the query portion

I think overloading merge is fine, since by Julia's definition in Base merge is defined on several dict-like objects, it doesn't have to be in the URI standard but more like a convenient one-liner defined in Julia - we can't show people what the standard is via interfaces I guess? to my understanding the URI specification is more about parsing not other things.

Base.merge(uri::URI, d::Dict) = URI(uri; query=merge(queryparams(uri), d))

I'll make a PR of the rest changes then.

@c42f
Copy link
Collaborator

c42f commented Nov 13, 2020

it doesn't have to be in the URI standard but more like a convenient one-liner defined in Julia

Very true. My thought was that we'd have to combine the existing query string with the new key value pairs, and that would be making a possibly invalid assumption about the format of that string. But perhaps that's not something to worry about.

@Roger-luo
Copy link
Contributor Author

umm, so what about just support this convenient one-liner first and see what happens?

@c42f
Copy link
Collaborator

c42f commented Nov 23, 2020

so what about just support this convenient one-liner first and see what happens?

Yeah I think that'd be ok. For merge we'd have to wait a deprecation cycle. To be honest, I think we could just name it mergequery() and be done with it.

@fonsp
Copy link
Member

fonsp commented May 19, 2022

merge is being deprecated for URI(::URI; kw...), right?

function Base.merge(uri::URI; kw...)
Base.depwarn("`merge(uri::URI; kw...)` is deprecated, use `URI(uri; kw...)` instead.", :merge)
return URI(uri; kw...)
end

@Roger-luo
Copy link
Contributor Author

yes, I think that's what I originally posted this issue for then I submitted a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants