Skip to content
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

Add extension methods for converting between maps and records #289

Merged
merged 2 commits into from
May 30, 2023

Conversation

nex3
Copy link
Member

@nex3 nex3 commented May 23, 2023

No description provided.

@lrhn
Copy link
Member

lrhn commented May 23, 2023

I'm not particularly fond of treating maps as sets of pairs.
And I'm not sure I see the use-cases. Where do these pairs come from?

On the other hand, if records had existed when we added MapEntry, we wouldn't have added MapEntry.
We might have used ({K key, V value}) as the type instead of (K, V), but I'm not even sure.
So this not like this is unreasonable code, it's just overlapping with the existing MapEntry API, which means we should think about how well they can, and should, co-exist.

Should users use one or the other?
Do we have a recommendation?
Should we have one? (That question applies whether we actually have one or not).
Why?

If this is just to save typing, I don't think it's particularly convincing.
If we get dart-lang/language#2563, then iterating over keys and values will just be:

 for (var (:key, :value) in map.entries) { ... }

and then doing

  for (var (key, value) in map.pairs) { ... }

isn't much of a saving. It's just an almost identical way to write almost the same thing, so users might just mix and confuse two.

(If I get to make MapEntry an inline class on ({K key, V value}), any performance concerns wrt. the MapEntry class vs. a record should go away.)

You should almost never have to create a MapEntry object any more, unless you are implementing the Map interface.
A valid reason to use Map.fromEntries or addEntries is if you already have the entries, probably from another map. If you need to create MapEntry's to pass to Map.fromEntries, you should likely just write a map literal, or add the key/value pair to the map directly.

Which means that the same should apply for creating pairs in order to create a map or add to a map.
You should already have the pairs from somewhere, but where is that?

So, not convinced this PR is solving a problem that isn't already either solved adequately by MapEntry, or is obsoleted by collection literals.

(One could also just have:

extension EntryPair<K, V> on MapEntry<K, V> { 
  (K, V) asPair() => (key, value);
}
extension PairEntry<K, V> on (K, V) { 
  MapEntry<K, V> asMapEntry() => MapEntry($1, $2);
}
extension EntryPairs<K, V> on Iterable<MapEntry<K, V>> {
  Iterable<(K, V)> asPairs() => this.map((entry) => entry.asPair());
}
extension PairEntries<K, V> on Iterable<(K, V)> {
  Iterable<MapEntry<K, V>> asMapEntries() => this.map((pair) => pair.asMapEntry());
}

and do the conversion to/from pairs as needed.)

@nex3
Copy link
Member Author

nex3 commented May 23, 2023

I've been converting a considerable amount of code to Dart 3 style, and I find this extremely useful (specifically Map.pairs; the rest are mostly there for consistency). Even if we got inferred object patterns, a big part of the value here is being able to choose your own names for the keys and values. It's much easier to write

for (var (args, callback) in overloads.pairs)

than either (:key, :value) (which removes the documentary value of the names and the ability to avoid conflicts in nested loops) or (key: args, value: callback) (which is a lot more text for zero added clarity). When you're working with a lot of maps, iterating through them is a very common operation, especially given that collection literals incentivize iteration over higher-order functions like Map.map(). Smoothing out that process is very valuable.

@lrhn
Copy link
Member

lrhn commented May 24, 2023

I'm not entirely convinced, but I think I'd be more likely to accept just:

extension ... on MapEntry<K,V> {
  (K, V) asPair() => (key, value);
}
extension ... on List<MapEntry<K, V>> {
  Iterable<(K, V)> asPairs() => map((e) => e.asPair());
}
// and just maybe:
extension ... on Map<K, V> {
  Iterable<(K, V)> get keyValuePairs => entries.asPairs();
}

I can see how that would make it easier to consume map key/value pairs as themselves, rather than seeing them only as actual map entries.

I'm not sold on using those pairs to build new maps again. There should be better ways to do that.

@natebosch WDYT?

@natebosch
Copy link
Member

I think I'd be more likely to accept just:

I don't think we should need asPair or asPairs - the only time I really see an Iterable<MapEntry> is Map.entries, and if we can go directly from the map to the pairs then the extra conversions aren't likely to have many use cases.

I would use the name entryPairs over keyValuePairs because it's a little shorter and consistent with entries. I'm iffy on the name pairs.

I'm not sold on using those pairs to build new maps again.

+1 - especially at the top level I don't think it's worth a method for creating a fresh map from pairs. I could go either way with addPairs - I'd like to see some real world use cases though.

I find this extremely useful (specifically Map.pairs; the rest are mostly there for consistency)

That's also the only API I would imagine should get much use. I don't think it's worth adding the others for consistency.

@nex3
Copy link
Member Author

nex3 commented May 24, 2023

I think if the expectation is that mapFromPairs() and Map.addPairs() aren't likely to be useful, it raises the question of whether Map.fromEntries() or Map.addEntries() are pulling their weight either, but I suppose that's a different conversation. I've removed them from this PR.

I'd like to gently push towards keeping the name .pairs, for two reasons. First, it matches .entries as a single pluralized word, which makes sense for a method that will probably largely subsume rather than coexist with uses of .entries. Second, the terseness is beneficial especially since for ... in lines can get fairly wide, particularly in nested collection literal context and when the assignment is destructuring a map pair into meaningful names.

@lrhn
Copy link
Member

lrhn commented May 24, 2023

it raises the question of whether Map.fromEntries() or Map.addEntries() are pulling their weight

They're not. If we didn't have them today, I wouldn't add them. The fromEntries constructor (and fromIterables, etc.) were basically all obsoleted by the more capable map literals. The addEntries has a theoretical use, but it's never actually been used much in practice. It is used, but just not very much.

And if records had existed then, we wouldn't have the MapEntry class. But we do, and .entries is harder to get rid of since it does provide useful functionality, and it's harder to ignore.

I can see the conciseness of .pairs, but it's also a completely nondescript name. Pairs of what?
Maybe documentation can help, maybe the type Iterable<(K, V)> makes it obvious. Maybe being declared in an extension named MapEntryKeyValuePairs will help.

But if I just saw fooIndex.pairs, with no context, I could only try to guess what it does.

(Mathematically, graph would be correct, but that's ... really not useful. And entries is taken.)

@nex3
Copy link
Member Author

nex3 commented May 24, 2023

I think pairs is about as self-explanatory as entries. A map is logically made up of key/value pairs in the same way it logically contains a set of entries (which are themselves key/value pairs). In some ways, it's clearer, since it explicitly indicates the structure of the data it returns where entries could be interpreted as containing a heterogeneous flat collection of all the keys and values.

@munificent
Copy link
Member

I can see the conciseness of .pairs, but it's also a completely nondescript name. Pairs of what?

I mean, List has first and last which are equally nondescript. They make sense because you know what you're calling it on: They return the first and last things in a list.

With a map, I think it's pretty obvious that the "pairs" in a map would be the key-value pairs. And the natural way to represent a pair in Dart 3 is a two-positional-field record. I think the name works pretty well.

@lrhn
Copy link
Member

lrhn commented May 26, 2023

A List is a sequence of elements (so is an Iterable). The "first" and "last" of a sequence of elements are elements. It's ... somewhat explainable.

A Map is a mapping from keys to values. Or, if one prefers, a set of map entries (key and value) with unique keys. The pairs then sounds like pairs of entries, or pairs of mappings. It's only if you already think of maps as "pairs of keys and values", which is a view I've actively tried to discourage, that the "pairs" is directly meaningful.

(I've not been particularly successful in discouraging that view, so maybe pairs will work fine for most people.)

@nex3
Copy link
Member Author

nex3 commented May 26, 2023

I'm not sure why you've tried to discourage the view of maps as "pairs of keys and values", but I think it's worth pointing out that the very first words in the Map documentation are "A collection of key/value pairs" so I think that battle may already be lost.

@lrhn
Copy link
Member

lrhn commented May 30, 2023

Fair. I'm really discouraging using a linked hash map instead of, what can to day be written as, a List<(K, V)>, and not actually using the map nature. Thinking of the map as its graph (set of pairs/points) is a valid perspective.

So a map is a set of pairings, but if someone say "the pairs of a map", the key/value pairs is not necessarily my first thought, that very much depend on the map. That may just be because I'm overthinking it.

I haven't found a better name than pairs, and everybody else seems to like it, so OK.

@natebosch natebosch merged commit 6ac3b5c into dart-lang:3.0 May 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants