<!DOCTYPE html>
<html>
<head>
<title></title>
</head>
<body><div style="font-family:Arial;">Responses inline.<br></div>
<div style="font-family:Arial;"><br></div>
<div id="sig40804545"><div class="signature"><span class="font" style="font-family:arial, sans-serif, sans-serif">Best,</span><span class="font" style="font-family:arial, sans-serif, sans-serif"></span><br></div>
<div class="signature"><span class="font" style="font-family:arial, sans-serif, sans-serif"> Zachary Waldowski</span><span class="font" style="font-family:arial, sans-serif, sans-serif"></span><br></div>
<div class="signature"><span class="font" style="font-family:arial, sans-serif, sans-serif"> </span><a href="mailto:zach@waldowski.me"><span class="font" style="font-family:arial, sans-serif, sans-serif">zach@waldowski.me</span></a><br></div>
</div>
<div><br></div>
<div><br></div>
<div>On Wed, Apr 5, 2017, at 08:45 PM, Ben Cohen via swift-evolution wrote:<br></div>
<blockquote type="cite"><div><div><div>• What is your evaluation of the proposal?<br></div>
</div>
</div>
</blockquote><div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Overall, +1. Dictionary and in Set are in need of interaction improvements. However, given the increased need for considering source stability, I'm of the opinion adding new stuff to the stdlib should face an appropriately high bar; so specific and nit-picky notes follow.<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">- The `init(_ keysAndValues: Sequence)` should be `init?(_:)`.<br></div>
<div style="font-family:Arial;">- The merging family does not feel completely in line with Swift naming. Considering other closure-taking methods in the stdlib, I feel like "by mergingValues" is a better fit, since "merg[ing]" already comes up in the signature, netting you `Dictionary(merging: listOfPairs, by: max)`.<br></div>
<div style="font-family:Arial;">- I'm not sure subscripts allow all this right now, but `default defaultValue` seems like an appropriate candidate for being `@autoclosure` and `throws`, and the subscript itself `rethrows`.<br></div>
<div style="font-family:Arial;">- I retract my concerns over the merging methods, but I still don't see the API benefit of `mapValues`. This doesn't seem like it pulls its weight besides an optimization. With SE-0154 and conditional conformances, I feel like the same optimization could be possible with `init(_ keysAndValues: Sequence)` and `lazy`. So -1 there.<br></div>
<div style="font-family:Arial;">- Source-breaking changes from `filter(_:)` overloads feel more problematic than described in the proposal. Both behaviors (returning Self and returning Array) are useful, and I don't want to lose easy calling of one. The desired behavior can also be accomplished using one of the initializers and `filter` or `lazy.filter`. -0.5 on that.<br></div>
<div style="font-family:Arial;">- As much as I want it personally, `grouped(by:)` feels like a weak specialization of `init(merging:mergingValues:)`. Now, of course, that would be better (for performance, readability, etc.) if `combine` were `inout`. This parallels nicely with the subscript-with-default being a mutating l-value. I don't remember how `reduce`-with-`inout` fell out back in January, but this feels like it's in similar territory. I just don't want to lock us into a collection of methods that are only slightly only different enough to be frustrating when using them.<br></div>
<div style="font-family:Arial;"><br></div>
<blockquote type="cite"><div><div><div>• Is the problem being addressed significant enough to warrant a change to Swift?<br></div>
</div>
</div>
</blockquote><div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">For sure. Arrays and generalized collections are best-in-class; the hashed collections are only slightly less so.<br></div>
<div style="font-family:Arial;"><br></div>
<blockquote type="cite"><div><div><div>• Does this proposal fit well with the feel and direction of Swift?<br></div>
</div>
</div>
</blockquote><div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">With some nits as noted above, yes.<br></div>
<div style="font-family:Arial;"><br></div>
<blockquote type="cite"><div><div><div>• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?<br></div>
</div>
</div>
</blockquote><div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">I, for one, don't feel like Dictionary needs to shoot much past the capabilities of NSMutableDictionary, but should definitely at least match it; this proposal gets us pretty close.<br></div>
<div style="font-family:Arial;"><br></div>
<blockquote type="cite"><div><div><div>• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?<br></div>
</div>
</div>
</blockquote><div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Followed the proposal since inception. Uh, big user of dictionaries, I suppose.<br></div>
<div style="font-family:Arial;"><br></div>
<blockquote type="cite"><div><div><div><br></div>
<div style="font-family:Arial;">More information about the Swift evolution process is available at <a href="https://github.com/apple/swift-evolution/blob/master/process.md">https://github.com/apple/swift-evolution/blob/master/process.md</a><br></div>
<div><div style="font-family:Arial;"><br></div>
<div><div style="font-family:Arial;">Thank you,<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Ben Cohen<br></div>
<div style="font-family:Arial;">Review Manager<br></div>
</div>
<div><br></div>
<div><br></div>
</div>
</div>
</div>
<div><u>_______________________________________________</u><br></div>
<div>swift-evolution mailing list<br></div>
<div><a href="mailto:swift-evolution@swift.org">swift-evolution@swift.org</a><br></div>
<div><a href="https://lists.swift.org/mailman/listinfo/swift-evolution">https://lists.swift.org/mailman/listinfo/swift-evolution</a><br></div>
</blockquote><div style="font-family:Arial;"><br></div>
</body>
</html>