Add Map#updateWith to modify a map based on current value #7320

exoego · 2018-10-06T03:15:54Z

Addresses scala/bug#11185 by adding the following methods that is analogus to java.util.Map.compute

-	~~compute~~ updateWith	~~computed~~ updatedWith
Added to	mutable.Map concurrent.Map	immutable.Map
Return value	New value `Option[V]`	Updated `Map[K,V]`

NthPortal · 2018-10-06T06:12:01Z

src/library/scala/collection/concurrent/Map.scala

+   *
+   * @param key the key value
+   * @param remappingFunction the function to compute a new mapping
+   * @tparam V1 the type of newly computed value


V1 does not appear to be used here

Oops, removed.

src/library/scala/collection/concurrent/Map.scala

julienrf · 2018-10-06T12:18:59Z

I’m not sure compute is a meaningful name (except for people familiar with the Java API). FTR, in scala-collection-contrib it’s named updateWith (updatedWith for the immutable version).

diesalbla · 2018-10-06T12:24:38Z

If it serves as an inspiration, in a Haskell library there is a similar method called alter.

exoego · 2018-10-06T13:35:16Z

IMO, this method should have a name that differ from mutable.Map#update and immutable.Map#updated. In contrast with update/updated that can only add or overwrite the key-value pair, this method can also remove the existing pair. In short, update + remove = new name.

alter may be good.
compute is my preference since I have a background of Java.

NthPortal · 2018-10-13T06:10:35Z

An interesting question is whether or not the argument should be a function (as it is now) or partial function (as it is in collection-contrib, linked by @julienrf)

SethTisue · 2018-11-06T16:24:47Z

I don't think we should call it compute, in many cases I'd be willing to just follow a Java precedent, but compute is an astonishingly terrible name, you could call practically anything compute

I think updateWith/updatedWith is fine

SethTisue · 2018-11-06T16:25:22Z

An interesting question is whether or not the argument should be a function (as it is now) or partial function

what are the arguments either way?

P.S. needs rebase
P.P.S. s/Abesent/Absent/g

julienrf · 2018-11-06T21:24:12Z

what are the arguments either way?

Version with a partial function

Definition site:

def updatedWith(key: K)(pf: PartialFunction[Option[V], Option[V]]): Map[K, V]

Call site:

map.updatedWith("foo") { case Some(previous) => Some(previous * 2) }

Version with a total function

Definition site:

def updatedWith(key: K)(pf: Option[V] => Option[V]): Map[K, V]

Call site:

map.updatedWith("foo") {
  case Some(previous) => Some(previous * 2)
  case None => None
}

The version with a partial function has a more complex type signature, but allows users to write less code at use site. Conversely, the version with a total function has a simpler type signature but requires users to always specify the cases they want to ignore.

Both versions are fine to me.

NthPortal · 2018-11-06T23:44:42Z

I agree with @julienrf - both are fine to me as well. I have a weak preference for the one which uses a PartialFunction, but would consider that outweighed if anyone has any performance arguments

NthPortal · 2018-11-06T23:46:29Z

I am okay with updateWith/updatedWith - despite the fact that update/updated cannot remove elements, I still think it is fairly intuitive. alter/altered is also a possibility, though I'm concerned that it is less intuitive. I would probably lean towards the former

exoego · 2018-11-07T01:26:43Z

I rebased the PR and renamed compute to updateWith.

Total function is used instead of PartialFunction.
IMHO, PartialFunction complexify updateWith. PF allows users not to write unnecessary case, but users should understand what happens in case of ignored case (e.g. MatchError thrown? or Existing value returned?).Total function version is easier for users to imagine and understand the behavior of updateWith.
Total functionalso allow users to write less code with underscore, e.g. _.orElse(Some(...)) or something like that.

szeiger · 2018-11-13T18:14:20Z

src/library/scala/collection/immutable/Map.scala

+    val previousValue = this.get(key)
+    val nextValue = remappingFunction(previousValue)
+    (previousValue, nextValue) match {
+      case (None, None) => this.asInstanceOf[CC[K,V1]]


this.asInstanceOf[CC[K,V1]] should be coll

szeiger · 2018-11-13T18:15:37Z

src/library/scala/collection/immutable/Map.scala

+    val nextValue = remappingFunction(previousValue)
+    (previousValue, nextValue) match {
+      case (None, None) => this.asInstanceOf[CC[K,V1]]
+      case (Some(_), None) => this.remove(key).asInstanceOf[CC[K,V1]]


We had a similar case recently. It would be good to restrict C <: CC[K, V] to get rid of this cast. I think I tried such a change a while ago but ran into some problems. It's worth another try.

julienrf · 2018-12-11T12:29:39Z

@exoego Would you have some time to address the last reviews?

exoego · 2018-12-11T13:16:12Z

@exoego Would you have some time to address the last reviews

I took several hours but still could not fix generics constraints. I think It is too complex for non-expert of scala collections.
I am ok if someone take over this PR .

julienrf · 2018-12-11T13:41:23Z

@exoego Yeah, it might be better to not change type bounds… I’ve pushed an update of the PR, rebased, and with Stefan’s comments addressed, in #7517 (I had to open another PR because I can not push force to your repo…). Thanks again for your contribution!

szeiger · 2018-12-11T16:24:30Z

Yes, ignore the type bounds. I gave it another try and didn't get any further than #7504. See my comments there on what the remaining problems are.

szeiger · 2018-12-14T16:29:02Z

I merged the updated version in #7517

scala-jenkins added this to the 2.13.0-RC1 milestone Oct 6, 2018

NthPortal reviewed Oct 6, 2018

View reviewed changes

src/library/scala/collection/concurrent/Map.scala Outdated Show resolved Hide resolved

exoego force-pushed the map-compute branch 3 times, most recently from d6f09d1 to 3f68a43 Compare October 6, 2018 12:17

SethTisue added WIP and removed WIP labels Nov 6, 2018

exoego force-pushed the map-compute branch from 5d006ab to 45d4be7 Compare November 7, 2018 00:49

exoego changed the title ~~Add Map#compute to modify a map based on current value~~ WIP: Add Map#compute to modify a map based on current value Nov 7, 2018

exoego force-pushed the map-compute branch from 45d4be7 to 046f34b Compare November 7, 2018 01:05

Add updateWith/updatedWith to update a map based on current value.

5406fd4

exoego force-pushed the map-compute branch from 046f34b to 5406fd4 Compare November 7, 2018 01:08

exoego changed the title ~~WIP: Add Map#compute to modify a map based on current value~~ Add Map#updateWith to modify a map based on current value Nov 7, 2018

SethTisue added the library:collections PRs involving changes to the standard collection library label Nov 8, 2018

szeiger reviewed Nov 13, 2018

View reviewed changes

julienrf mentioned this pull request Dec 11, 2018

Add updateWith/updatedWith to update a map based on current value. #7517

Merged

szeiger closed this Dec 14, 2018

SethTisue removed this from the 2.13.0-RC1 milestone Feb 22, 2019

exoego deleted the map-compute branch June 27, 2019 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Map#updateWith to modify a map based on current value #7320

Add Map#updateWith to modify a map based on current value #7320

exoego commented Oct 6, 2018 •

edited

Loading

NthPortal Oct 6, 2018

exoego Oct 6, 2018

julienrf commented Oct 6, 2018

diesalbla commented Oct 6, 2018 •

edited

Loading

exoego commented Oct 6, 2018 •

edited

Loading

NthPortal commented Oct 13, 2018

SethTisue commented Nov 6, 2018 •

edited

Loading

SethTisue commented Nov 6, 2018 •

edited

Loading

julienrf commented Nov 6, 2018

NthPortal commented Nov 6, 2018

NthPortal commented Nov 6, 2018

exoego commented Nov 7, 2018 •

edited

Loading

szeiger Nov 13, 2018

szeiger Nov 13, 2018

julienrf commented Dec 11, 2018

exoego commented Dec 11, 2018 •

edited

Loading

julienrf commented Dec 11, 2018 •

edited

Loading

szeiger commented Dec 11, 2018

szeiger commented Dec 14, 2018

Add Map#updateWith to modify a map based on current value #7320

Add Map#updateWith to modify a map based on current value #7320

Conversation

exoego commented Oct 6, 2018 • edited Loading

NthPortal Oct 6, 2018

Choose a reason for hiding this comment

exoego Oct 6, 2018

Choose a reason for hiding this comment

julienrf commented Oct 6, 2018

diesalbla commented Oct 6, 2018 • edited Loading

exoego commented Oct 6, 2018 • edited Loading

NthPortal commented Oct 13, 2018

SethTisue commented Nov 6, 2018 • edited Loading

SethTisue commented Nov 6, 2018 • edited Loading

julienrf commented Nov 6, 2018

Version with a partial function

Version with a total function

NthPortal commented Nov 6, 2018

NthPortal commented Nov 6, 2018

exoego commented Nov 7, 2018 • edited Loading

szeiger Nov 13, 2018

Choose a reason for hiding this comment

szeiger Nov 13, 2018

Choose a reason for hiding this comment

julienrf commented Dec 11, 2018

exoego commented Dec 11, 2018 • edited Loading

julienrf commented Dec 11, 2018 • edited Loading

szeiger commented Dec 11, 2018

szeiger commented Dec 14, 2018

exoego commented Oct 6, 2018 •

edited

Loading

diesalbla commented Oct 6, 2018 •

edited

Loading

exoego commented Oct 6, 2018 •

edited

Loading

SethTisue commented Nov 6, 2018 •

edited

Loading

SethTisue commented Nov 6, 2018 •

edited

Loading

exoego commented Nov 7, 2018 •

edited

Loading

exoego commented Dec 11, 2018 •

edited

Loading

julienrf commented Dec 11, 2018 •

edited

Loading