dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.06k stars 1.56k forks source link

Add a Map#merge function #2644

Closed DartBot closed 9 years ago

DartBot commented 12 years ago

This issue was originally filed by @MarkBennett


What steps will reproduce the problem?

  1. Create a map called A. (Such as one of request parameter defaults.)
  2. Use Map#forEach() to merge values from another map, B, into A.
  3. Get annoyed when doing this many times.

What is the expected output? What do you see instead?

The Map interface should define a merge function which merges together two Maps.

What version of the product are you using? On what operating system?

Version 0.1.0.201204121423, Build 6479 Dart SDK version 6478, Dartium version

madsager commented 12 years ago

Added Area-Library, Triaged labels.

DartBot commented 12 years ago

This comment was originally written by @seaneagan


I think both mutative and non-mutative versions would be useful:

// mutative void putAll(Map<K, V> other); // non-mutative Map<K, V> merge(Map<K, V> other);

However, an Object#clone would make non-mutative versions of APIs slightly less necessary:

var merged = map.clone().putAll(otherMap);

Though I'm not sure whether cloning an Map/List/etc. should produce another immutable object in which case this wouldn't work.

DartBot commented 12 years ago

This comment was originally written by @seaneagan


Nevermind about Object#clone, looks like someone with much more experience has discoverered that copy constructors are superior:

http://www.artima.com/intv/bloch13.html

So my example would instead be:

var merged = new Map.from(map).putAll(otherMap);

which isn't so bad.

lrhn commented 11 years ago

Issue #6415 has been merged into this issue.


cc @nex3.

justinfagnani commented 11 years ago

FYI: HashMap and LinkedHashMap both have an addAll() method, but Map does not. The analyzer just starting reporting warnings for objects typed Map, and it looked like to a lot of people that Map had just removed addAll()


Set owner to @floitschG. Added C1 label.

lrhn commented 11 years ago

I must admit I thought Map had an addAll method, which is why I implemented it in HashMap.

If we add addAll, we should also consider having addMissing, where addAll uses the argument map's value if both have the key, and addMissing uses the original value.

DartBot commented 11 years ago

This comment was originally written by @seaneagan


instead of:

map.addMissing(otherMap);

in many cases one could mutate otherMap:

otherMap.addAll(map);

or create a new Map:

var merged = new Map.from(otherMap)..addAll(map);

floitschG commented 11 years ago

I agree. No need to implement addMissing. If we really need it, we can add it later.

I don't like the addAll name, since All usually stands for Iterables, but it's too late to change this now.

Ok to add addAll to the interface, but at the same time we need to adapt SplayTreeMap (which doesn't have addAll yet.


Added Ready-to-implement, Accepted labels.

lrhn commented 11 years ago

Added Fixed label.