Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager. When contacting the review manager directly, please put "SE-0510" at the start of the subject line.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
Yes to both counts, it is great to smooth away performance issues while at the same time simplifying the api surface for what in my experience is the most common use case.
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I followed the original pitch discussion and did a quick reading of both the proposal and actual implementation PR.
Because the parameters are ordered (Key, Value) rather than (Value, Key)—correctly, in my view—I still think mapKeyedValues or some variant thereof is more conducive to helping users "hold it right" than mapValuesForKeys (most salient, obviously, in the case where Key == Value).
This view was strengthened by the pitch phase discussion that either parameter order is a plausible design (and thereby a plausible point of user confusion)—among other reasons, because the (Value, Key) order would be more source compatible for existing mapValues closures, since $0 would continue to mean the same thing—and by the pointing out of some existing Objective-C API which does swap the parameter order to align with function name.
Additionally, in the newly described future direction where the current mapValues gets a different name in some future Swift version so that unqualified mapValues can be aliased to the new API, mapUnkeyedValues would be a nicer name for the original than mapValuesWithoutKeys in my view.
I agree with this and the previous discussion about mapValuesWithKeys being suggestive of the "wrong" order of parameters.
Would mapValuesByKey work better than mapValuesWithKeys? To, me, "by" suggests "it gives you each key and its value", while "with" suggests "it gives you each value and its key".
I think this is an obvious improvement to the standard library. I don't feel strongly about the name (mapValuesWithKeys, mapValuesForKeys, mapKeyedValues, or otherwise) but I do wonder if the ergonomics there could be improved by some future language feature that requires labels on closure arguments, so you would (hypothetically) be required to write something like:
dict.map { key x, value y in
...
}
I'm not proposing we do that here, just wondering aloud if some new syntax would solve the ergonomics problem of the argument swappage.
Maybe. I'm not proposing any language changes here, just wondering if it's a language-level problem rather than an API design problem. Happy to bikeshed more in DMs if you like, but I don't want to derail this review.
If we are adding `mapValuesWithKeys` as a variation of `mapValues`, I believe we should also add `compactMapValuesWithKeys` as a variation of `compactMapValues`.
The proposal states that:
`compactMapValues` is essentially a shorthand for calling `reduce(into:)`
However, personally, I find this way of thinking difficult to arrive at intuitively.
When I'm building logic and naturally try to use `mapValues`, I sometimes realize that some values might disappear in the process, which leads me to want to use `compactMapValues`. In those moments, I have never once thought, "Well, I could just write this with `reduce` instead."
Furthermore, I felt the logic was unnatural.
This seems to be a "reason why `compactMapValues` is unnecessary" rather than a "reason why `compactMapValues` should not have the same variations as `mapValues`."
That is something that should have been debated when `compactMapValues` was first introduced. Since it has already been implemented, wouldn't it be more valuable to maintain consistency based on its existence?
I like this addition. The performance is a very valid motivation, but the ergonomics and readability are as well. In this respect, I agree with @omochimetaru that a compact-map version of mapValuesWithKeys would be just as relevant. While the equivalent of compactMap() can be implemented with reduce(), the power of reduce() is so much greater that we intentionally implement compactMap() for its clarity.
I don't feel strongly about the name (