groupByMap naming
ljharb opened this issue · 30 comments
flatMap means ".flat after .map". It kind of seems like groupByMap should mean ".groupBy after .map", by that precedent.
I'm not sure I have a better naming suggestion, but perhaps it's worth bikeshedding?
.groupByMap means that the result is grouped and placed to the Map and it's how it works by the spec. I don't think that the names collision of .map method and Map constructor should be resolved in the scope of this proposal.
I do understand how it works. I'm saying that someone unfamiliar with the spec who is familiar with .map, .flat, and .flatMap and sees the name might be confused about what .groupByMap does, especially since .groupBy exists along with .map.
We're changing from groupByMap to groupByToMap
@jridgewell what means To in this case? If that means "pass to Map constructor (or at least somehow convert to Map) the result of .groupBy" - it's incorrect since the result of .groupBy is an object and can't store such keys that are used in the resulting map. That causes more confusion than .groupByMap.
The way it was phrased during discussion was that we're converting "groupings" to a Map, not the object return value from groupBy. Essentially it's like we had "group by to Object" as groupBy and "group by to Map" as groupByToMap. This naming was overwhelmingly preferred by the delegates.
In this case, it's unrelated to flatMap comparison and this issue. Ok, however, I still think that ...ByTo... is terrible for this case.
Map.fromArray(array, groupFunction)
Object.fromArray(array, groupFunction)
?
@zloirock I agree it's awkward, but when "group by" is followed by a noun, it usually means "group by {noun}" literally - "group by name", "group by size", etc. - so group-by-map reads as "group by map" rather than "group by" + "(map variant of this method)" absent prior context.
@bathos so maybe that means that by is unnecessary or unsuitable?
.groupByToMap contains 4 words - and still confusing. Sure, it's not the only method from the JS standard library that contains 4+ words, however, IIRC all the rest of such methods are low level, like .getOwnPropertyNames, but for high-level code, we have one-word name .keys instead of .getOwnPropertyEnumerableNames. .groupByToMap suggests frequent use in the high-level code.
I’m pretty confident grouping and producing a Map will be a fairly uncommon use case; it’s just an important one in those rare cases.
The “by” was considered critical to keep by multiple delegates, because “group by” has an existing connotation about what the callback returns.
I’m pretty confident grouping and producing a Map will be a fairly uncommon use case
I'm just one developer working in one team and can't say how common or uncommon this is, but as a little piece of anecdotal evidence, I'll mention that we already use a Map-producing groupBy utility function in our codebase (instead of, say, lodash.groupBy) and I'll probably use the Map-producing version of these two Array methods almost exclusively.
The even/odd example is one where the object-producing version shines because the produced keys are well known and simple. But for me, grouping by (semi-)arbitrary user inputted data is a much more compelling and common use case, and there Maps are quite useful.
I don't have strong opinions about the naming, though, just wanted to put that out there. groupByToMap seems fine to me.
I’m pretty confident grouping and producing a Map will be a fairly uncommon use case; it’s just an important one in those rare cases.
I have cases for a such method several times for a month.
The “by” was considered critical to keep by multiple delegates, because “group by” has an existing connotation about what the callback returns.
We can say almost the same about most of Array methods - for example, why .reduce or .filter haven't such clarifications in their names?
because neither of those return a key name, which is what the "by" strongly implies.
I agree this will probably be common, which is why I asked for it. I don't know why @ljharb imagines otherwise.
As to the names: reduce and filter are the normal names of those methods in other languages and in JavaScript libraries. groupBy is the normal name of this method. I don't see the contradiction.
(OK, sometimes reduce is called foldLeft, but reduce is still a pretty common name. Besides, they're existing methods, which can't change.)
@ljharb .filter is a subset of .groupBy. Why in the method name is not specified what will be filtered, like in .filterReject? Why in the .reduce name is not specified how and to what the array should be reduced? The name and the signature of .reduce could cause more issues than .group.
@bakkot those names, even .groupBy, were fine for me - but after this renaming .groupBy is a reason for .groupByToMap - it's not a common method name and it looks ugly. Removing By could fix it.
filter is not a subset of groupBy. it doesn't do any grouping at all. filter takes a predicate, which answers the question "should i keep this item?". groupBy returns a property key that items are grouped by.
Not a true subset, but it seems fair to say filter is a kind of "grouping": arr.groupByToMap(predicate).get(true)?
However filter seems like an argument for keeping "by" because it's not exactly intuitive that you return true to say don't filter.
@bathos that you can do every array method with reduce doesn't mean they're all a form of reducing :-p
The name of the operation is still "groupBy". There is the default one and there is the one which goes to a Map. Hence "groupBy" and "groupByToMap". This seems fine to me. Removing the "by" would be confusing because it is part of the name of the operation.
Ok, I will not impose my opinion further if others are against it.
@ljharb .filter is a kind of grouping anyway and how it works makes no meaning.
I think what's notable in this case is that you can get a result that includes the filter result with the same callback you would pass to filter itself @ljharb, which is not like everything-can-be-reduce, but I agree it's not particularly significant either way.
I know that this issue was closed as if there's nothing left to discuss and everybody else's opinion is unimportant, but groupByToMap() is such a weird sounding function name to me. I'm sure that's something I'm always going to end up forgetting and being unsure about. Would groupToMapBy() maybe be better? It flows a bit better in English.
The name of the operation is "groupBy"; I think any name which doesn't keep that as a substring is not a good choice.
I agree with the groupByX naming scheme. I think the groupByToMap is a bit wordy, but that's relatively low priority for proposals. The core complaint here is that the original groupByMap naming can be confused with performing a .map + .groupBy exactly like the OP describes. A few committee members had the same misunderstanding, they thought we were implementing a .map + .groupBy instead of a groupBy into a Map. The new naming was extremely favorable during the meeting, because it reduced the confusinion at the expense of an additional word. That's a pretty good trade-off in my opinion.
I think further bikesheding the name is unlikely to produce a better phrase, and I don't think the new name is so terrible that we should delay implementations (the proposal is now Stage 3, the implementation stage).
FWIW I would have called it groupIntoMap() or groupAsMap().
I don't expect these names to be used. But this is a bikeshedding thread so I'm just dropping them here so they can be said to have been considered and rejected.
I actually think @fisker is onto something and Javascript has a problem about how and where type conversion happens. But that's off topic. These are useful functions, whatever they end up being called. *wanders into the sunset muttering something about quotients and equivalence classes...*
Out of curiosity, was there any consideration for making this particular method a factory method on the Map class?
I imagine something like Map.createGroupBy(array, groupFunc) mostly keeps the desired naming scheme while also indicating the creation of a Map instance.
Perhaps it is less discoverable not as an array method, but if this functionality is only intended to meet a few rare use cases, discoverability doesn't seem that important.
This could also allow it to work with any iterable (object supporting the Symbol.iterator method), rather than just something array-like. So something like: Map.createGroupBy(iterable, groupFunc).
Not sure if I even like this, but throwing this in here that there could be [Symbol.createGroupBy](iterable, groupFunc), with default implementations existing both in Object[Symbol.createGroupBy] and Map[Symbol.createGroupBy] (and why not WeakMap too), and then the Array.prototype.groupBy would take an object with [Symbol.createGroupBy] as optional second argument, defaulting to Object if it's not provided. I.e. the groupByToMap would instead become
arr.groupBy(groupFunc, Map)if this functionality is only intended to meet a few rare use cases, discoverability doesn't seem that important.
I don't think there is consensus on that aspect: for some, use might be rare and for others, use might be common. I don't think discoverability would be significantly reduced if the returns-a-Map variant were associated with the Map constructor.
This could also allow it to work with any iterable (object supporting the
Symbol.iteratormethod), rather than just something array-like. So something like:Map.createGroupBy(iterable, groupFunc).
I would expect something like Map.createGroupBy(iterable, groupFunc) to only pass the value as an argument to groupFunc.
If the groupFunc callback would be called with the index of the element as an argument then it seems more consistent for groupByToMap to be a method on the Array.prototype (and be transferable to other array-like objects).
I would expect something like
Map.createGroupBy(iterable, groupFunc)to only pass the value as an argument togroupFunc.
That's a fair point. I thought the index could still be useful in some situations and easily determined from storing and incrementing a value during iteration, but you're right that it's associated more with ordered data. Either way, it's not the most inconvenient thing to spread the iterable data to an array first and then pass it to the function. In hindsight, maybe that's preferable because it enforces the input type to be the same as (array-like) the type the elements are grouped into.
If the
groupFunccallback would be called with the index of the element as an argument then it seems more consistent forgroupByToMapto be a method on theArray.prototype(and be transferable to other array-like objects).
As mentioned above, I think providing the index of the element just indicates that the function should act on an array somehow. However, it feels a bit reductionist to conclude that it must then belong on the Array.prototype.
My concern is that it feels a bit strange to have .groupByToMap() return a different, unrelated complex data type (a Map). With the exception of .reduce() which can return anything, no other array method seems to do that. To me, it kind of gives the feeling that the Map is created out of thin air.
On the other hand, with a factory method on the Map class, Map.createGroupBy(array, groupFunc) clearly indicates what kind of "Map" we are talking about and what data is being used and created.
My concern is that it feels a bit strange to have
.groupByToMap()return a different, unrelated complex data type (aMap).
That kind of interaction between different collection types is normal in the standard libraries of other languages; for example, java.util.Map#keySet() returns a java.util.Set. I think JavaScript users will quickly get used to it, and I would find it a bit strange to set a precedent that methods of one JavaScript collection type should not return an instance of a different collection type.
Of course, that doesn't mean the returns-a-Map variant must then belong on the Array.prototype. But, given the callback is passed a value, index, object being traversed, and an optional thisArg, then the returns-a-Map variant is substantially consistent with other Array.prototype methods.