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 the review manager @Ben_Cohen (via email or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
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?
This seems to be a reasonable and useful addition to the standard library. The problem being addressed is clearly stated, and the solution definitely fits well with the rest of the Swift standard library. I followed the initial discussion and re-read the proposal.
+1 Followed initial discussion and read this proposal.
It fills an obvious gap with a name appropriate solution that's inline with a number of mainstream languages.
compactMapValues seems like a natural fit for Swift. It provides measurable utility for conversion from [T: U?] -> [T: U] and for selecting non-nil [T: V] entries from [T: f(U) -> V?]. It is one of those useful utilities that I've written several times and would love to see be built-in.
I like the current push to include useful features like this. It builds on SE-0165, which has been a godsend (Thank you @nnnnnnnn) .
Yes, I believe it fits well with the feel and direction of Swift.
Looks good to me. In future I would also like to see a compactValues() method on Dictionary, and a matching compact() on Array, to complete the set here.
Is the problem being addressed significant enough to warrant a change to Swift?
It's a useful method, and I wouldn't hesitate to include compactMapValues in a dedicated collection-library... but when we talk about Swift itself, I don't think it fulfills the requirements:
A method name that consists of more than two words imho is already a good indicator that a functionality isn't really fundamental, and as others pointed out before (Add compactMapValues to Dictionary - #11 by Jon_Hull), the implementation is quite obvious when you are not married to the idea of using fp exclusively.
Of course, including compactMapValues in the stdlib wouldn't hurt anyone... but there has to be a higher bar when we want to keep the stdlib lean (although I'm not sure if that is an official goal).
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
It fills an obvious hole in the API. Like others here, I have written this several times for myself, and it is just one of those base things that should be included.
Sure, this is mentioned in the proposal as .compactMapValues {$0}. I just think it might be nicer as a separate method where you don't need to mentally remove the “map” part after seeing the identity function part. For similar reasons, I don't think having the default argument be id would be great, though it is a possibility.
I agree, it would seem really strange to see a call to a method with map in its name and not see a transform. If we want this behavior directy there should be a separate method named compactValues.
Point taken and Agreed.
If anything the implementation for these could be .compactMap(id) and .compactMapValues(id) -- where id is most likely replaced by the KeyPath equivalent of id.
This was discussed at the time of the name change of compactMap. compact cannot be expressed in the generics system yet. In fact it's used, under the name someValues, as the motivating example for parameterized extensions in the generics manifesto.
Luckily we don't have to debate it, because it isn't a possibility. You can't default a generic argument in that way. Try it by compiling the following:
func id<T>(_ t: T) -> T { return t }
extension Sequence {
func cMap<T>(_ transform: (Element)->T? = id) -> [T] {
return reduce(into: []) {
if let x = transform($1) { $0.append(x) }
}
}
}
There are temporary practical reasons prior to ABI stability for keeping the standard library small, because it must be shipped with every app. Once the library ships with the OS, this is no longer a concern.
Method proliferation can be a problem. If every type is bristling with overlapping or barely-useful methods, the API becomes bewildering and hard to use. This is more of a concern when extending broad protocols like Collection, less so when extending concrete types like Dictionary.
Consistency is also a mitigating factor here. If there is a map on Sequence and a mapValues on Dictionary, then it is helpful to users working with the API to follow through with compactMap and compactMapValues.
Obviousness of implementation is not necessarily a reason to exclude an addition. There are many methods in the standard library that have an obvious implementation. "Trivially composable" has been used as the term for avoiding gratuitous additions – but the word trivial is important here. It refers to compositions such as !isEmpty rather than operations that require a for loop or multiple compound statements.
Some criteria to consider for an addition of a method to the standard library are:
does it aid readability?
is it a common operation?
is it trivially composable?
is it consistent with existing methods?
does it help avoid a correctness trap?
does it help avoid a performance trap?
alternatively: might it encourage misuse?
can it be more efficiently implemented internally within the std lib?
It's trivial considering id works with .compactMap but if we add an additional criteria point: (lower the bar for newcomers); then surely there's merit?
I may be missing something, but this extension seems to work just fine:
extension Sequence {
func compact<Dummy>() -> [Dummy] where Element == Dummy? {
return compactMap({$0})
}
}
let xs = [1, 2, nil, 4, 5]
let ys = xs.compact()
print(ys) // [1, 2, 4, 5]
(One definite usability downside is ys.compact() will still show up in autocompletion and the resulting error is rather unfriendly: "Generic parameter 'Dummy' could not be inferred".)