{Map, Keyword}.replace/3 and replace!/3 #5917

Qqwy · 2017-03-23T12:50:03Z

Culmination of this discussion on the Elixir Forum.

~~This Pull Request adds the following:~~

Map.put_new!/3 which raises a KeyExistsError if the key that should be inserted already exists.
Map.put_existing/3 which only puts a value in the given map if the key already existed.
Map.put_existing!/3 which only puts a value in the given map if the key already existed, and raises a KeyError if it did not.
Keyword.put_new!/3 which raises a KeyExistsError if the key that should be inserted already exists.
Keyword.put_existing/3 which only puts a value in the given keyword list if the key already existed.
Keyword.put_existing!/3 which only puts a value in the given keyword list if the key already existed, and raises a KeyError if it did not.
The afore-mentioned KeyExistsError exception module.
Documentation + Doctests for the above.

Note that Keyword.put_existing and Keyword.put_existing! add a new value to the front of the keyword list without removing the original one. I expect that this is the behaviour we want, but I wanted to draw attention to it to make sure.

After discussion below, this is changed to:

Map.replace/3 which only puts a value in the given map if the key already existed.
Map.replace!/3 which only puts a value in the given map if the key already existed, and raises a KeyError if it did not.
Keyword.replace/3 which only puts a value in the given keyword list if the key already existed. All earlier values with the same key are deleted from the keyword list.
Keyword.replace!/3 which only puts a value in the given keyword list if the key already existed, and raises a KeyError if it did not. All earlier values with the same key are deleted from the keyword list.

…/3, Keyword.put_new!/3.

lexmag · 2017-03-23T13:03:05Z

put_new and put_new! pair behaves differently from !-paired functions, I'm not sure if it's good thing to break expectations in that case.
Let's probably have separate PR with put_new! addition so we can review and merge put_existing!/3 separately?

lexmag · 2017-03-23T13:05:30Z

lib/elixir/lib/keyword.ex

+  def put_new!(keywords, key, value) when is_list(keywords) and is_atom(key) do
+    case :lists.keyfind(key, 1, keywords) do
+      {^key, _} -> raise KeyExistsError, key: key, term: keywords
+      false     -> [{key, value} | keywords]


Please use single whitespace before ->. (https://github.com/lexmag/elixir-style-guide#expression-group-alignment)

👍, I really have a hard time with the lining up of = or -> like this. It makes incidental change a common place. If we want to add a new condition or change the name of a var we end up having to respect the entire group.

michalmuskala · 2017-03-23T13:09:03Z

lib/elixir/lib/keyword.ex

+      {^key, _} -> [{key, value} | keywords]
+      false     -> keywords
+    end
+  end


Shouldn't this update the value, instead of adding a new pair?

adkron

Overall I think the code looks great. I'd love to see some tests around it, or if we are using doctests I think we have an issues with iex> Keyword.put_existing([a: 1], :b, 2).

I'm not sure that I find exceptions to be that helpful in this case. I would rather have it return an :ok tuple or and :error tuple. Exceptions bubble up and I'm not sure how this would be an exceptional circumstance. Do you have a use case that shows why we need the exceptions?

adkron · 2017-03-23T13:05:44Z

lib/elixir/lib/keyword.ex

+  ## Examples
+
+  iex> Keyword.put_existing([a: 1], :b, 2)
+  [a: 1]


The example output needs to have the new key, right?

Nevermind, I was confused. I now see that this is correct.

michalmuskala · 2017-03-23T13:10:15Z

lib/elixir/lib/map.ex

+  def put_existing!(map, key, value) do
+    case has_key?(map, key) do
+      true  -> put(map, key, value)
+      false -> raise KeyError, key: key, term: map


This is equivalent to the :maps.update/3 BIF

Yes, we should use the bif and inline it accordingly.

So would this become:

case :maps.update(map, key, new_value) do {:badkey, key} -> raise KeyError, key: key, term: map map -> map end

?

michalmuskala · 2017-03-23T13:11:27Z

lib/elixir/lib/map.ex

+  """
+  @spec put_existing(map, key, value) :: map
+  def put_existing(map, key, value) do
+    case has_key?(map, key) do


I think it will be better to use the :maps.update/3 instad of :maps.put/3 BIF in here - it gives the runtime additional knowledge, we expect the key to be there, so it has more opportunities for optimisation.

josevalim · 2017-03-23T13:12:18Z

One option we are considering is to rename put_existing to replace, put_existing! to replace! and no longer add put_new!. Any preference on the name?

Qqwy · 2017-03-23T13:14:31Z

@josevalim @michalmuskala I expect that the naming difference from put_existing and replace also describes the difference in behaviour between what the Keyword implementation should do:

For Keyword.put_existing it makes the most sense to me to put a new version of an already-existing key at the front of the list. (instead of replacing the value that existed further down the list).
For Keyword.replace it makes the most sense to me to replace the value where it is in the list, rather than to add a new tuple at the front.

josevalim · 2017-03-23T13:17:41Z

@Qqwy if that is your opinion, it means we should go with replace rather than put_existing.

adkron · 2017-03-23T13:19:36Z

I prefer replace as I think I wouldn't have thought the docs were wrong with that wording. I think it is both concise and precise. 👍

michalmuskala · 2017-03-23T13:19:38Z

I like replace more than put_existing, and I don't feel a huge need for put_new! - put_new itself is already rather rare use case, this one would be even rarer.

lexmag · 2017-03-23T13:22:21Z

I don't feel a huge need for put_new! - put_new itself is already rather rare use case, this one would be even rarer.

Agree on this.

I like put_existing though, it forms nice group of put* functions and assists in searching.

michalmuskala · 2017-03-23T13:25:37Z

@Qqwy I think the behaviour of the corresponding functions in Map and Keyword should be such that:

map |> Map.foo(...) |> Map.to_list == map |> Map.to_list |> Keyword.foo(...)

holds, of course ignoring ordering issues and non-atom keyed maps.

Qqwy · 2017-03-23T13:26:42Z

All right, I'll rewrite it to be Maps.replace/3 and Keyword.replace/3. Keyword.replace/3 will replace the value in the keyword list, rather than pushing a new element to the front.

Other changes to be done:

Use :maps.update/3 for Map.replace/3 as @michalmuskala observed 👍 .

Would the function of that body then become:

case :maps.update(map, key, new_value) do
  {:badkey, key} -> raise KeyError, key: key, term: map
  map -> map
end

?

Things to be discussed:

Yes/no about adding put_new!.
Maybe it is not something that should be discussed as part of this PR, but if we pass a struct to replace (or put_existing), would it be possible to raise a compile-time error if a non-existing field name is used?

@adkron Because the non-! versions of the functions already return a 'plain' value, it is impossible to work with tuple-results and have the bang-versions return plain values + exceptions instead. So our hand is forced in that regard because of backwards-compatibility.

josevalim · 2017-03-23T13:28:34Z

Ok, so let's go with replace and replace! and drop put_new!. Sorry for the back and forth @Qqwy.

Would the function of that body then become:

No. :maps.update already raises. :maps.update(key, val, map) is all you need.

Yes/no about adding put_new!.

No.

Maybe it is not something that should be discussed as part of this PR, but if we pass a struct to replace (or put_existing), would it be possible to raise a compile-time error if a non-existing field name is used?

No.

Qqwy · 2017-03-23T13:29:18Z

@michalmuskala I agree that for the majority of cases, this is a nice property to respect, but AFAIK the reason that Keyword exists, is to allow the possibility of having duplicate entries.

Qqwy · 2017-03-23T13:31:30Z

Sorry for the back and forth @Qqwy

No worries! That's what Pull Requests are for, right? Having a distilled snippet of code that can be reviewed in a more practical way than a general proposal can.

And: Thanks! I misread the Erlang documentation. I thought it returned a tuple, rather than raising a tuple.

Qqwy · 2017-03-23T13:40:19Z

Next question about Keyword.replace/3: Say we do the following:

[a: 1, b: 2, a: 3] 
|> Keyword.replace(:a, 4)

What should the output now be?

[a: 4, b: 2, a: 3]
[a: 4, b: 2]

- Map.put_existing/3 to Map.replace, - Map.put_existing!/3 to Map.replace! - uses :maps.update/3 underwater for the implementation of these two functions. - Removes Maps.put_new!/3

michalmuskala · 2017-03-23T13:53:02Z

The same as for Keyword.put, just raising if the key is not there.

adkron · 2017-03-23T13:58:00Z

@Qqwy, I would expect both to be replaced. I don't like it, but that is my expectation.

- Removes Keyword.put_new!/3 - Replaces Keyword.put_existing/3 with Keyword.replace/3 - Replaces Keyword.put_existing!/3 with Keyword.replace!/3 - Behaviour of the Keyword.replace/3 and Keyword.replace!/3 functions is altered so they delete _all_ other occurrences of an existing value.

kelvinst · 2017-03-23T14:03:14Z

lib/elixir/lib/exception.ex

  end
 end

+defmodule KeyExistsError do


The name KeyExistsError feels "strange" to me. Maybe DuplicatedKeyError, but still a little bit strange. IDK 😕

This is going to be removed again, as it was only required for put_new/3.

OvermindDL1 · 2017-03-23T14:39:35Z

Keyword lists should always be pre-pended to when adding or replacing something, it should not remove later ones, I am relying on that in quite a few areas.

Qqwy · 2017-03-23T14:52:45Z

The current implementation follows the precedence set by Map.put as @michalmuskala noted for consistency's-sake, but I agree with @OvermindDL1 that only altering the front of the keyword list feels like it is the proper behaviour for keyword lists.

OvermindDL1 · 2017-03-23T15:08:23Z

The current implementation follows the precedence set by Map.put as @michalmuskala noted for consistency's-sake, but I agree with @OvermindDL1 that only altering the front of the keyword list feels like it is the proper behaviour for keyword lists.

It is the erlang way for proplists (which yes I know elixir calls them keyword lists but they've seemed conceptually similar, except requiring atom keys only), and I very much am relying on it in more than a few places... ^.^;

Remember that Erlang's proplists have not only a get_value/2 call but also a get_all_values/2 call, where the first returns the first match and the second returns all matches in the entire proplist, this is highly useful functionality in many areas (like where I am using it for Elixir's Keyword lists).

(Side-note, erlang property lists also support not only {Key, Value} but also some_atom, which is short-hand for {some_atom, true}, I miss this shorthand in elixir, I use a lot of blah: true to compensate, but unsure how the syntax would work considering elixir's parser, hmm... EDIT: Maybe just supporting blah: inline as in foo(42, blorp: 86, blah:, floom: 21), or foo(blah:)?)

Qqwy · 2017-03-23T15:10:46Z

@OvermindDL1 There both is Keyword.get (newest value) and Keyword.get_values (all values) in the Keyword module.

lexmag · 2017-03-23T15:05:52Z

lib/elixir/lib/map.ex

+
+  ## Examples
+
+  iex> Map.replace(%{a: 1}, :b, 2)


Examples need 4 spaces indentation.

lexmag · 2017-03-23T15:06:00Z

lib/elixir/lib/keyword.ex

+  later occurrences are removed.
+
+  ## Examples
+


Examples need 4 spaces indentation.

OvermindDL1 · 2017-03-23T15:13:28Z

@OvermindDL1 There both is Keyword.get (newest value) and Keyword.get_values (all values) in the Keyword module.

Exactly, hence why I am saying that put_existing/3/replace/3 should prepend a value, those old values are needed sometimes as it is a multi-value map in essence. I could see the use of adding a replace_all/3 that removes all matching keys then prepends the new one on to the list though.

whatyouhide · 2017-03-23T15:34:32Z

My 2 cents: replace is clearer in what it does than put_existing. put_* is already present yes, but put_existing just reads funny compared to the so clear replace :).

OvermindDL1 · 2017-03-23T15:38:47Z

I'd opt for replace for Map, but put_existing would be more accurate for Keyword I'd think.

EDIT: Actually put_existing/replace might not be necessary at all for Keyword as you can just put it on anyway, but replace_all would be useful.

josevalim · 2017-03-23T16:24:25Z

@Qqwy we only need to address @lexmag comments about indentation and this is good to go!

Qqwy · 2017-03-23T17:15:38Z

@lexmag There we go!

josevalim · 2017-03-23T17:52:54Z

❤️ 💚 💙 💛 💜

Qqwy added 3 commits March 23, 2017 13:06

implement Map.put_existing/3, Map.put_existing!/3, Map.put_new!/3.

6ada8f8

Fix doctests; implement Keyword.put_existing/3, Keyword.put_existing!…

8767dfe

…/3, Keyword.put_new!/3.

Snip!

feafd8f

lexmag reviewed Mar 23, 2017

View reviewed changes

michalmuskala reviewed Mar 23, 2017

View reviewed changes

adkron reviewed Mar 23, 2017

View reviewed changes

michalmuskala reviewed Mar 23, 2017

View reviewed changes

Remove carefully entered spaces, based on @lexmag's feedback ;-)

caaad8c

Qqwy force-pushed the put_existing branch from da28621 to caaad8c Compare March 23, 2017 13:11

- Renames:

951b6c0

- Map.put_existing/3 to Map.replace, - Map.put_existing!/3 to Map.replace! - uses :maps.update/3 underwater for the implementation of these two functions. - Removes Maps.put_new!/3

kelvinst approved these changes Mar 23, 2017

View reviewed changes

lexmag suggested changes Mar 23, 2017

View reviewed changes

josevalim changed the title ~~{Map, Keyword}.put_existing/3, put_existing!/3 and put_new!/3~~ {Map, Keyword}.replace/3 and replace!/3 Mar 23, 2017

Fix indentation of doctests.

3ef2402

josevalim merged commit 3f88ab7 into elixir-lang:master Mar 23, 2017

{Map, Keyword}.replace/3 and replace!/3 #5917

{Map, Keyword}.replace/3 and replace!/3 #5917

Uh oh!

Conversation

Qqwy commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lexmag commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adkron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Mar 23, 2017

Uh oh!

Qqwy commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josevalim commented Mar 23, 2017

Uh oh!

adkron commented Mar 23, 2017

Uh oh!

michalmuskala commented Mar 23, 2017

Uh oh!

lexmag commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalmuskala commented Mar 23, 2017

Uh oh!

Qqwy commented Mar 23, 2017

Other changes to be done:

Things to be discussed:

Uh oh!

josevalim commented Mar 23, 2017

Uh oh!

Qqwy commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qqwy commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qqwy commented Mar 23, 2017

Uh oh!

michalmuskala commented Mar 23, 2017

Uh oh!

adkron commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OvermindDL1 commented Mar 23, 2017

Uh oh!

Qqwy commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OvermindDL1 commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qqwy commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qqwy commented Mar 23, 2017 •

edited

Loading

Qqwy commented Mar 23, 2017 •

edited

Loading

lexmag commented Mar 23, 2017 •

edited

Loading

Qqwy commented Mar 23, 2017 •

edited

Loading

Qqwy commented Mar 23, 2017 •

edited

Loading

Qqwy commented Mar 23, 2017 •

edited

Loading

OvermindDL1 commented Mar 23, 2017 •

edited

Loading

OvermindDL1 commented Mar 23, 2017 •

edited

Loading

Qqwy commented Mar 23, 2017 •

edited

Loading