-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Raise when given non-keyword-lists in Keyword.merge/3 #5479
Raise when given non-keyword-lists in Keyword.merge/3 #5479
Conversation
Can we get another pair of eyes on this one? |
Looking into it right now. |
assert Keyword.merge([a: 1, b: 2, c: 3, c: 4], [c: 11, c: 50, d: 13], fun) == [a: 1, b: 2, c: 14, c: 54, d: 13] | ||
end | ||
|
||
test "merge/2 and merge/3 should work exactly the same way" do |
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.
Let's not use "should", wdyt? We can say "merge/2 and merge/3 behave in the same way" or something like that.
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.
fixed
Keyword.merge(arg1, arg2, fun) | ||
end | ||
end | ||
end |
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.
As far as I understand, in this part we're testing that if merge/2
raises an ArgumentError
, then merge/3
should raise that same error, right? If so, wdyt if we write it like this:
for {arg1, arg2} <- args_error do
error = assert_raise ArgumentError, ~r/expected a keyword list/, fn -> Keyword.merge(arg1, arg2) end
assert_raise ArgumentError, error.message, fn -> Keyword.merge(arg1, arg2, fun) end
end
It should work since all terms in args_error
will error so the try
feels superfluous.
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.
i didn't realize assert_raise
would return the exception struct.
addressed!
Similar to elixir-lang#5466 It makes sure a valid keyword is given as both arguments. Additionally, it adds tests to make sure Keyword.merge/2 and /3 work exactly the same way. Related issue: elixir-lang#5395
suggestions addressed |
5f9da2d
to
b969582
Compare
Thanks @eksperimental 💟 |
Similar to
#5466
It makes sure a valid keyword is given as both arguments.
Additionally, it adds tests to make sure Keyword.merge/2 and /3
work exactly the same way.
Related issue:
#5395