Skip to content

Conversation

@janderland
Copy link
Contributor

Added guards to ensure only lists can be passed to Keywords.merge/3.

I thought about adding this change after realizing the merge/3 method had inconsistent behavior...

Keyword.merge([a: 1, b: 2], nil)
# ** (FunctionClauseError)

Keyword.merge([], nil)
# nil

What do you think?

Added guards to ensure only lists can be passed to Keywords.merge/3.
@yordis
Copy link
Contributor

yordis commented Dec 29, 2018

@janderland it seems that it should always expect two Keywordlist instead of simply return one or the other one.

This is an odd behavior from API perspective to be honest.

@eksperimental
Copy link
Contributor

I agree with the changes, but if you want to be correct, the first two clauses should be removed all together, and let the third clause deal with it.

The current implementation and the proposed one, will not pass this test:

iex> Keyword.merge([:this, :is, :not, :a, :kw, :list], [])
[:this, :is, :not, :a, :kw, :list]

@michalmuskala
Copy link
Member

Those two clauses are an important optimisation for when merging with an empty keyword. In general, the Keyword module does not guarantee in only accepts valid keywords - it might accept other things as well, but it must accept valid keywords. It checks when it is practical or does not impose performance issues, but not otherwise.

@josevalim josevalim merged commit e3dacf0 into elixir-lang:master Dec 29, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

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.

5 participants