Skip to content

URI.merge/2#4644

Merged
josevalim merged 9 commits into
elixir-lang:masterfrom
andrewtimberlake:feature/uri_merge
May 20, 2016
Merged

URI.merge/2#4644
josevalim merged 9 commits into
elixir-lang:masterfrom
andrewtimberlake:feature/uri_merge

Conversation

@andrewtimberlake
Copy link
Copy Markdown
Contributor

Merge a relative path/URI to a base URI as per RFC3986 §5.2

Examples:

iex> URI.merge("http://example.com/foo", "/bar") |> to_string
"http://example.com/bar"

iex> URI.parse("http://example.com/foo") |> URI.merge("bar") |> to_string
"http://example.com/bar"

…and more

Comment thread lib/elixir/lib/uri.ex Outdated
iex> URI.merge("http://example.com", "http://google.com")|> to_string
"http://google.com"
"""
def merge(%URI{authority: nil}, %URI{}), do: raise ArgumentError, "You must merge onto an absolute URI"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error messages start with lower case.

@josevalim
Copy link
Copy Markdown
Member

Thank you @andrewtimberlake. I have added some comments. I also believe we should call this function URI.join. I would expect a function named merge to merge all fields, so I think join reflects it a bit better.

@andrewtimberlake
Copy link
Copy Markdown
Contributor Author

@josevalim I believe it is merging all fields as per the RFC which refers to this as a merge.
“The pseudocode above refers to a "merge" routine for merging a
relative-path reference with the path of the base URI”

@josevalim
Copy link
Copy Markdown
Member

josevalim commented May 19, 2016

Right. We need to consider though the meaning "merge" has in Elixir and the meaning it has in the UFC. If we want to call it "merge", then I would recommend us to call it merge_path.

@andrewtimberlake
Copy link
Copy Markdown
Contributor Author

I understand. I don’t want to belabour the point but we are doing more than just merging the path.

URI.merge("http://example.com", "http://google.com") |> to_string == "http://google.com"
URI.merge("http://example.com/foo?a=b", "?c=d") |> to_string == "http://example.com/foo?c=d"
URI.merge("http://example.com/foo?a=b", "") |> to_string == "http://example.com/foo?a=b"

From that info, you tell me what you’d like me to name it.

@josevalim
Copy link
Copy Markdown
Member

Merge it is. :)

Comment thread lib/elixir/lib/uri.ex Outdated
"http://google.com"
"""
def merge(%URI{authority: nil}, %URI{}), do: raise ArgumentError, "you must merge onto an absolute URI"
def merge(%URI{}, %URI{scheme: scheme}=rel) when not is_nil(scheme) do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always have spaces around =.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I’ll update

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think scheme != nil (instead of not is_nil(scheme)) reads more easily.

@josevalim
Copy link
Copy Markdown
Member

@andrewtimberlake please ping us when this is ready to go. :)

Comment thread lib/elixir/lib/uri.ex Outdated
|> Enum.join("/")
end

defp remove_dot_segments(list, acc \\ [])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can ditch this clause and simply call remove_dot_segments with [] on L#410.

@andrewtimberlake
Copy link
Copy Markdown
Contributor Author

@josevalim It’s ready to go if you’re happy.

@josevalim josevalim merged commit ede00a1 into elixir-lang:master May 20, 2016
@andrewtimberlake andrewtimberlake deleted the feature/uri_merge branch May 20, 2016 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants