SE-165: Dictionary & Set Enhancements


(Ben Cohen) #1

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
  https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

  Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

  Reply text

  Other replies

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 https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager


(Jon Hull) #2

+1000

I have written many of these for my own uses (e.g. group(by:)), and will be happy to have the standard library provide more efficient implementations than I can provide. I will now also be able to use these in frameworks without fear of stepping on others' implementations of the same thing.

I agree with Xiaodi’s renaming: ‘merge(_: by:)’ or maybe ‘merge(with: by:)’. But don’t let bikeshedding stop you from accepting this proposal, as I will take it whatever the wording...

Thanks,
Jon

···

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
  https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

  Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

  Reply text

  Other replies

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 https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Jarod Long) #3

• What is your evaluation of the proposal?

I think it's great! Enthusiastic +1 from me.

Just an idea -- in addition to `mapValues`, it seems like it would be useful to have both a `mapKeys` and plain `map` for key-value pairs. The proposal mentions that `map` was omitted because unique keys aren't guaranteed, but it seems reasonable to me to trap if duplicate keys are generated during a map, similar to how the proposed sequence-based initializer traps on duplicate keys.

I don't think those additions are critical, but they seem nice to me.

• Is the problem being addressed significant enough to warrant a change to Swift?

Yes -- I've struggled with several of the issues that the proposal addresses. The specialized map / filter, default subscript values, and merging are particularly appealing to me.

• Does this proposal fit well with the feel and direction of Swift?

I think so, since Swift already goes to great lengths to provide a robust collection system. This is a logical iteration of that idea.

• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I don't recall any features I've used elsewhere that would be helpful to compare to.

• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I didn't follow any discussion that led to the proposal, but I read the whole thing.

Jarod

···

On Apr 5, 2017, 17:45 -0700, Ben Cohen via swift-evolution <swift-evolution@swift.org>, wrote:

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reply text

Other replies

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 https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Brent Royal-Gordon) #4

  • What is your evaluation of the proposal?

(As a meta issue, I'm not sure I like the grab-bag review style; I'm finding this proposal a little bit difficult to navigate.)

Sequence-based initializer and merging initializer

Good idea, but I think these two are redundant with each other, and I don't think "merging" is an accurate way to describe what the second one does. (`merging` would suggest to me that it was combining several dictionaries or lists, not combining conflicting elements.) I'd suggest a single initializer along the lines of:

  init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy resolveConflict: (Value, Value) throws -> Value = { fatalError("Duplicate values \($0) and \($1)") }) rethrows
    where S.Iterator.Element == (key: Key, value: Value)

Merging methods

Good idea, but I'm not a fan of the `mergingValues:` label. I would suggest the same `correctingConflictsBy resolveConflict:` label I suggested for the previous method—possibly including the default value. I also think `merge(_:correctingConflictsBy:)`'s first parameter should be labeled `with`, just as the `merged` variant is.

I wonder if we might also want a method that copies the Dictionary, but with a single key added/removed/changed:

  func withValue(_ value: Value?, forKey key: Key) -> [Key: Value]

Key-based subscript with default value

I like the functionality, but not way this proposal does it. I don't like having the default value be a labeled parameter to the subscript, because it isn't used to locate the value. However, I can't come up with a better syntax without adding language features. What I'd like to do is make it possible to assign through `??`:

  frequencies[c] ?? 0 += 1

But that would require either that we support `inout` functions, or that `??` become magic syntax instead of a standard library feature. The former is not coming in Swift 4 and the latter is less than ideal.

Still, if we would rather have that syntax and we think we'll soon have the language improvements needed to pull it off, I'd suggest rejecting this portion of the proposal.

Dictionary-specific map and filter

I am +114 on this. I say that because I have received 114 upvotes on my Stack Overflow answer explaining how to write a `Dictionary.map` method: <http://stackoverflow.com/questions/24116271/whats-the-cleanest-way-of-applying-map-to-a-dictionary-in-swift/24219069#24219069>

I agree with the decision not to pass keys to the closures in these methods; that keeps them simple and focused, and ensures they stay parallel with ordinary `map` and `filter`. I also agree with the decision to not build in a form of `map` which allows key remapping; you can always do that with the sequence-based initializer, which would let you add conflict-handling logic more elegantly than a key-value `map` could.

Visible dictionary capacity

I doubt I'll ever use the `capacity` property, but I'm not opposed to it. Adding a `reserveCapacity(_:)` method is a good idea.

A grouped(by:) method for sequences

Yes, please.

Apply relevant changes to Set

These make sense. (I considered suggesting the `filter` method be called `intersection(where:)`, but on second thought, I don't think that conveys the actual use cases for the method very well.)

I wonder if we might want to conform `Dictionary` to `SetAlgebra`. They have compatible semantics, and I've occasionally wanted to use them in the same places. On the other hand, some of the methods might form attractive nuisances; perhaps I should just write a SetAlgebra-conforming view when I want to do that.

General notes

If SE-0161 is accepted, we may want to support key path variants of some of these APIs (like `grouped(by:)`, `map(_:)`, and `filter(_:)`). On the other hand, we may want to defer that so we can consider that issue holistically, including with existing Collection APIs.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. These are all common needs; when working with dictionaries, I find myself writing `for` loops with `var`s far more often than I'd like.

  • Does this proposal fit well with the feel and direction of Swift?

Yes, these are all pretty straightforward additions.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby's `Hash` has many of these features and I appreciate them there; `NSDictionary` does not and it suffers for it. Perl hashes have a flattening behavior that tends to be amenable to editing them in various ways, but that's not really suitable for Swift.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

···

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

--
Brent Royal-Gordon
Architechies


(Ricardo Parada) #5

+1

The only comment I have is the mergingValues argument label for the closure passed in to the merge methods.
My preference would be resolvingCollisionsWith:, i.e. merge(_:resolvingCollisionsWith:) and merged(with:resolvingCollisionsWith:).

···

On Apr 5, 2017, at 8:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
  https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
  https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

  Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

  Reply text

  Other replies

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 https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Xiaodi Wu) #6

This is an excellent proposal and a worthy addition. The problems addressed
are significant and these changes in general fit well with the direction of
Swift. One nit below, but otherwise I think this will be pretty great.

Nit:

merge(_:mergingValues:) and friends should be more consistent with other
closure labels as revised in SE-0118. Some options:

merge(_:mergingValuesBy:) // the "by" is used consistently in similar
closure labels
merge(_:combiningValuesBy:) // avoids repeating the word "merge"
merge(_:makingValueWith:) // by analogy with
ManagedBuffer.create(minimumCapacity:makingValueWith:)
merge(_:by:) // as with earlier versions of SE-0118, worth considering if
the more verbose labels "do much to enhance readability at the point of use"

···

On Wed, Apr 5, 2017 at 19:45 Ben Cohen via swift-evolution < swift-evolution@swift.org> wrote:

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs
through next Tuesday, April 11th. The proposal is available here:
https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews
should be sent to the swift-evolution mailing list at
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the
review manager. When replying, please try to keep the proposal link at the
top of the message:

Proposal link:
https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reply text

Other replies

*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
https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(James Berry) #7

+1. These are needed and well-considered enhancements.

Hello, Swift community!

The review of "SE-165: Dictionary & Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
  https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md
  • What is your evaluation of the proposal?

+1

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes

  • Does this proposal fit well with the feel and direction of Swift?

Yes

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

These enhancements bring swift closer to expectations for broad and orthogonal support for common operations, while making allowance for implementation efficiency.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

detailed, though not line-by-line.

···

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:


(Zachary Waldowski) #8

Responses inline.

Best,

  Zachary Waldowski

  zach@waldowski.me

• What is your evaluation of the proposal?

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.

- The `init(_ keysAndValues: Sequence)` should be `init?(_:)`.

- 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)`.
- 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`.
- 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.
- 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.
- 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.

• Is the problem being addressed significant enough to warrant a
change to Swift?

For sure. Arrays and generalized collections are best-in-class; the
hashed collections are only slightly less so.

• Does this proposal fit well with the feel and direction of Swift?

With some nits as noted above, yes.

• If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?

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.

• How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?

Followed the proposal since inception. Uh, big user of dictionaries,
I suppose.

···

On Wed, Apr 5, 2017, at 08:45 PM, Ben Cohen via swift-evolution wrote:

More information about the Swift evolution process is available at
https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen

Review Manager

_________________________________________________

swift-evolution mailing list

swift-evolution@swift.org

https://lists.swift.org/mailman/listinfo/swift-evolution


(David Beck) #9

Hello, Swift community!

The review of "SE-165: Dictionary&Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reply text

Other replies

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?

This looks great! I’ve definitely written a few of these methods myself, and if they were available in the standard library, I’d probably reach for them more often instead of less elegant solutions.

• Is the problem being addressed significant enough to warrant a change to Swift?

The language could survive without these changes, but if made, would prove useful.

• Does this proposal fit well with the feel and direction of Swift?

Yes.

• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby has a lot of these kinds of tools and they have been useful.

• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

a quick reading

More information about the Swift evolution process is available at https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager

David Beck
http://davidbeck.co
http://twitter.com/davbeck
http://facebook.com/davbeck


(Brad Hilton) #10

+1. These are great, swifty additions to the standard library. Good job Nate Cook!

···

Hello, Swift community!

The review of "SE-165: Dictionary&Set Enhancements" begins now and runs through next Tuesday, April 11th. The proposal is available here:
https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
https://lists.swift.org/mailman/listinfo/swift-evolution

or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link: https://github.com/apple/swift-evolution/blob/master/proposals/0165-dict.md

Reply text

Other replies

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 https://github.com/apple/swift-evolution/blob/master/process.md

Thank you,

Ben Cohen
Review Manager


(Nate Cook) #11

  • What is your evaluation of the proposal?

(As a meta issue, I'm not sure I like the grab-bag review style; I'm finding this proposal a little bit difficult to navigate.)

Sequence-based initializer and merging initializer

Good idea, but I think these two are redundant with each other, and I don't think "merging" is an accurate way to describe what the second one does. (`merging` would suggest to me that it was combining several dictionaries or lists, not combining conflicting elements.) I'd suggest a single initializer along the lines of:

  init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy resolveConflict: (Value, Value) throws -> Value = { fatalError("Duplicate values \($0) and \($1)") }) rethrows
    where S.Iterator.Element == (key: Key, value: Value)

Thanks for all your feedback, Brent! One note on this item in particular—if you specify a default argument for a throws/rethrows closure, you have to use "try" at the call site even if the default closure argument doesn't throw. Modules currently don't promise that default closure arguments don't throw, and a default argument could change from non-throwing to throwing in a later version of a library.

There's a bug tracking the issue here: https://bugs.swift.org/browse/SR-2979

···

On Apr 5, 2017, at 9:43 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Merging methods

Good idea, but I'm not a fan of the `mergingValues:` label. I would suggest the same `correctingConflictsBy resolveConflict:` label I suggested for the previous method—possibly including the default value. I also think `merge(_:correctingConflictsBy:)`'s first parameter should be labeled `with`, just as the `merged` variant is.

I wonder if we might also want a method that copies the Dictionary, but with a single key added/removed/changed:

  func withValue(_ value: Value?, forKey key: Key) -> [Key: Value]

Key-based subscript with default value

I like the functionality, but not way this proposal does it. I don't like having the default value be a labeled parameter to the subscript, because it isn't used to locate the value. However, I can't come up with a better syntax without adding language features. What I'd like to do is make it possible to assign through `??`:

  frequencies[c] ?? 0 += 1

But that would require either that we support `inout` functions, or that `??` become magic syntax instead of a standard library feature. The former is not coming in Swift 4 and the latter is less than ideal.

Still, if we would rather have that syntax and we think we'll soon have the language improvements needed to pull it off, I'd suggest rejecting this portion of the proposal.

Dictionary-specific map and filter

I am +114 on this. I say that because I have received 114 upvotes on my Stack Overflow answer explaining how to write a `Dictionary.map` method: <http://stackoverflow.com/questions/24116271/whats-the-cleanest-way-of-applying-map-to-a-dictionary-in-swift/24219069#24219069>

I agree with the decision not to pass keys to the closures in these methods; that keeps them simple and focused, and ensures they stay parallel with ordinary `map` and `filter`. I also agree with the decision to not build in a form of `map` which allows key remapping; you can always do that with the sequence-based initializer, which would let you add conflict-handling logic more elegantly than a key-value `map` could.

Visible dictionary capacity

I doubt I'll ever use the `capacity` property, but I'm not opposed to it. Adding a `reserveCapacity(_:)` method is a good idea.

A grouped(by:) method for sequences

Yes, please.

Apply relevant changes to Set

These make sense. (I considered suggesting the `filter` method be called `intersection(where:)`, but on second thought, I don't think that conveys the actual use cases for the method very well.)

I wonder if we might want to conform `Dictionary` to `SetAlgebra`. They have compatible semantics, and I've occasionally wanted to use them in the same places. On the other hand, some of the methods might form attractive nuisances; perhaps I should just write a SetAlgebra-conforming view when I want to do that.

General notes

If SE-0161 is accepted, we may want to support key path variants of some of these APIs (like `grouped(by:)`, `map(_:)`, and `filter(_:)`). On the other hand, we may want to defer that so we can consider that issue holistically, including with existing Collection APIs.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. These are all common needs; when working with dictionaries, I find myself writing `for` loops with `var`s far more often than I'd like.

  • Does this proposal fit well with the feel and direction of Swift?

Yes, these are all pretty straightforward additions.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby's `Hash` has many of these features and I appreciate them there; `NSDictionary` does not and it suffers for it. Perl hashes have a flattening behavior that tends to be amenable to editing them in various ways, but that's not really suitable for Swift.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Ricardo Parada) #12

How important is it that the closure be allowed to throw when resolving collisions? In my use cases merging dictionaries he resolving of the collisions is always relatively simple. Shouldn’t they be as below maybe?

    /// Creates a new dictionary using the key/value pairs in the given sequence,
    /// using a combining closure to determine the value for any duplicate keys.
    init<S: Sequence>(
        merging keysAndValues: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element

    /// Merges the key/value pairs in the given sequence into the dictionary,
    /// using a combining closure to determine the value for any duplicate keys.
    mutating func merge<S: Sequence>(
        _ other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element
    /// Returns a new dictionary created by merging the key/value pairs in the
    /// given sequence into the dictionary, using a combining closure to determine
    /// the value for any duplicate keys.
    func merged<S: Sequence>(
        with other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) -> [Key: Value] where S.Iterator.Element == Element

···

On Apr 6, 2017, at 12:51 PM, Nate Cook via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 9:43 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

  • What is your evaluation of the proposal?

(As a meta issue, I'm not sure I like the grab-bag review style; I'm finding this proposal a little bit difficult to navigate.)

Sequence-based initializer and merging initializer

Good idea, but I think these two are redundant with each other, and I don't think "merging" is an accurate way to describe what the second one does. (`merging` would suggest to me that it was combining several dictionaries or lists, not combining conflicting elements.) I'd suggest a single initializer along the lines of:

  init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy resolveConflict: (Value, Value) throws -> Value = { fatalError("Duplicate values \($0) and \($1)") }) rethrows
    where S.Iterator.Element == (key: Key, value: Value)

Thanks for all your feedback, Brent! One note on this item in particular—if you specify a default argument for a throws/rethrows closure, you have to use "try" at the call site even if the default closure argument doesn't throw. Modules currently don't promise that default closure arguments don't throw, and a default argument could change from non-throwing to throwing in a later version of a library.

There's a bug tracking the issue here: https://bugs.swift.org/browse/SR-2979

Merging methods

Good idea, but I'm not a fan of the `mergingValues:` label. I would suggest the same `correctingConflictsBy resolveConflict:` label I suggested for the previous method—possibly including the default value. I also think `merge(_:correctingConflictsBy:)`'s first parameter should be labeled `with`, just as the `merged` variant is.

I wonder if we might also want a method that copies the Dictionary, but with a single key added/removed/changed:

  func withValue(_ value: Value?, forKey key: Key) -> [Key: Value]

Key-based subscript with default value

I like the functionality, but not way this proposal does it. I don't like having the default value be a labeled parameter to the subscript, because it isn't used to locate the value. However, I can't come up with a better syntax without adding language features. What I'd like to do is make it possible to assign through `??`:

  frequencies[c] ?? 0 += 1

But that would require either that we support `inout` functions, or that `??` become magic syntax instead of a standard library feature. The former is not coming in Swift 4 and the latter is less than ideal.

Still, if we would rather have that syntax and we think we'll soon have the language improvements needed to pull it off, I'd suggest rejecting this portion of the proposal.

Dictionary-specific map and filter

I am +114 on this. I say that because I have received 114 upvotes on my Stack Overflow answer explaining how to write a `Dictionary.map` method: <http://stackoverflow.com/questions/24116271/whats-the-cleanest-way-of-applying-map-to-a-dictionary-in-swift/24219069#24219069>

I agree with the decision not to pass keys to the closures in these methods; that keeps them simple and focused, and ensures they stay parallel with ordinary `map` and `filter`. I also agree with the decision to not build in a form of `map` which allows key remapping; you can always do that with the sequence-based initializer, which would let you add conflict-handling logic more elegantly than a key-value `map` could.

Visible dictionary capacity

I doubt I'll ever use the `capacity` property, but I'm not opposed to it. Adding a `reserveCapacity(_:)` method is a good idea.

A grouped(by:) method for sequences

Yes, please.

Apply relevant changes to Set

These make sense. (I considered suggesting the `filter` method be called `intersection(where:)`, but on second thought, I don't think that conveys the actual use cases for the method very well.)

I wonder if we might want to conform `Dictionary` to `SetAlgebra`. They have compatible semantics, and I've occasionally wanted to use them in the same places. On the other hand, some of the methods might form attractive nuisances; perhaps I should just write a SetAlgebra-conforming view when I want to do that.

General notes

If SE-0161 is accepted, we may want to support key path variants of some of these APIs (like `grouped(by:)`, `map(_:)`, and `filter(_:)`). On the other hand, we may want to defer that so we can consider that issue holistically, including with existing Collection APIs.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. These are all common needs; when working with dictionaries, I find myself writing `for` loops with `var`s far more often than I'd like.

  • Does this proposal fit well with the feel and direction of Swift?

Yes, these are all pretty straightforward additions.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby's `Hash` has many of these features and I appreciate them there; `NSDictionary` does not and it suffers for it. Perl hashes have a flattening behavior that tends to be amenable to editing them in various ways, but that's not really suitable for Swift.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Brent Royal-Gordon) #13

Ugh. If that's the case, I'd probably provide a merge-handler-argument-less version instead of using a default argument. But I would have the first argument of both be unlabeled.

Actually, if we're going to do that, maybe the non-merge-handling version should be failable. That's the one semantic a `resolveConflict` method can't provide, and of course trapping on error is still just one force-unwrap away if that's what you want.

So that would give us:

  init?<S: Sequence>(_ keysAndValues: S)
    where S.Iterator.Element == (key: Key, value: Value)

  init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy resolveConflict: (Value, Value) throws -> Value) rethrows
    where S.Iterator.Element == (key: Key, value: Value)

I think that pretty much covers everything you could possibly want to do about a conflict.

(I'd do a similar thing to `merged`, and perhaps have `merge` return a Bool, if you don't give them `resolveConflict` functions.)

···

On Apr 6, 2017, at 9:51 AM, Nate Cook <nate@natecook.com> wrote:

Thanks for all your feedback, Brent! One note on this item in particular—if you specify a default argument for a throws/rethrows closure, you have to use "try" at the call site even if the default closure argument doesn't throw. Modules currently don't promise that default closure arguments don't throw, and a default argument could change from non-throwing to throwing in a later version of a library.

There's a bug tracking the issue here: https://bugs.swift.org/browse/SR-2979

--
Brent Royal-Gordon
Architechies


(Shawn Erickson) #14

They rethrow which is IMHO perfect for this. If the closure throws then the
outer functions throws (e.g. requires try). This allows folks that have a
need to throw if they want without forcing it on everyone.

···

On Thu, Apr 6, 2017 at 6:53 PM Ricardo Parada via swift-evolution < swift-evolution@swift.org> wrote:

How important is it that the closure be allowed to throw when resolving
collisions? In my use cases merging dictionaries he resolving of the
collisions is always relatively simple. Shouldn’t they be as below maybe?

    /// Creates a new dictionary using the key/value pairs in the given sequence, /// using a combining closure to determine the value for any duplicate keys. init<S: Sequence>(
        merging keysAndValues: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element

    /// Merges the key/value pairs in the given sequence into the dictionary, /// using a combining closure to determine the value for any duplicate keys. mutating func merge<S: Sequence>(
        _ other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element

    /// Returns a new dictionary created by merging the key/value pairs in the /// given sequence into the dictionary, using a combining closure to determine /// the value for any duplicate keys. func merged<S: Sequence>(
        with other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) -> [Key: Value] where S.Iterator.Element == Element

On Apr 6, 2017, at 12:51 PM, Nate Cook via swift-evolution < > swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 9:43 PM, Brent Royal-Gordon via swift-evolution < > swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution < > swift-evolution@swift.org> wrote:

• What is your evaluation of the proposal?

(As a meta issue, I'm not sure I like the grab-bag review style; I'm
finding this proposal a little bit difficult to navigate.)

*Sequence-based initializer and merging initializer*

Good idea, but I think these two are redundant with each other, and I
don't think "merging" is an accurate way to describe what the second one
does. (`merging` would suggest to me that it was combining several
dictionaries or lists, not combining conflicting elements.) I'd suggest a
single initializer along the lines of:

init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy
resolveConflict: (Value, Value) throws -> Value = { fatalError("Duplicate
values \($0) and \($1)") }) rethrows
where S.Iterator.Element == (key: Key, value: Value)

Thanks for all your feedback, Brent! One note on this item in
particular—if you specify a default argument for a throws/rethrows closure,
you have to use "try" at the call site even if the default closure argument
doesn't throw. Modules currently don't promise that default closure
arguments don't throw, and a default argument could change from
non-throwing to throwing in a later version of a library.

There's a bug tracking the issue here:
https://bugs.swift.org/browse/SR-2979

*Merging methods*

Good idea, but I'm not a fan of the `mergingValues:` label. I would
suggest the same `correctingConflictsBy resolveConflict:` label I suggested
for the previous method—possibly including the default value. I also think
`merge(_:correctingConflictsBy:)`'s first parameter should be labeled
`with`, just as the `merged` variant is.

I wonder if we might also want a method that copies the Dictionary, but
with a single key added/removed/changed:

func withValue(_ value: Value?, forKey key: Key) -> [Key: Value]

*Key-based subscript with default value*

I like the functionality, but not way this proposal does it. I don't like
having the default value be a labeled parameter to the subscript, because
it isn't used to locate the value. However, I can't come up with a better
syntax without adding language features. What I'd like to do is make it
possible to assign through `??`:

frequencies[c] ?? 0 += 1

But that would require either that we support `inout` functions, or that
`??` become magic syntax instead of a standard library feature. The former
is not coming in Swift 4 and the latter is less than ideal.

Still, if we would rather have that syntax and we think we'll soon have
the language improvements needed to pull it off, I'd suggest rejecting this
portion of the proposal.

*Dictionary-specific map and filter*

I am +114 on this. I say that because I have received 114 upvotes on my
Stack Overflow answer explaining how to write a `Dictionary.map` method: <
http://stackoverflow.com/questions/24116271/whats-the-cleanest-way-of-applying-map-to-a-dictionary-in-swift/24219069#24219069
>

I agree with the decision not to pass keys to the closures in these
methods; that keeps them simple and focused, and ensures they stay parallel
with ordinary `map` and `filter`. I also agree with the decision to not
build in a form of `map` which allows key remapping; you can always do that
with the sequence-based initializer, which would let you add
conflict-handling logic more elegantly than a key-value `map` could.

*Visible dictionary capacity*

I doubt I'll ever use the `capacity` property, but I'm not opposed to it.
Adding a `reserveCapacity(_:)` method is a good idea.

*A grouped(by:) method for sequences*

Yes, please.

*Apply relevant changes to Set*

These make sense. (I considered suggesting the `filter` method be called
`intersection(where:)`, but on second thought, I don't think that conveys
the actual use cases for the method very well.)

I wonder if we might want to conform `Dictionary` to `SetAlgebra`. They
have compatible semantics, and I've occasionally wanted to use them in the
same places. On the other hand, some of the methods might form attractive
nuisances; perhaps I should just write a SetAlgebra-conforming view when I
want to do that.

*General notes*

If SE-0161 is accepted, we may want to support key path variants of some
of these APIs (like `grouped(by:)`, `map(_:)`, and `filter(_:)`). On the
other hand, we may want to defer that so we can consider that issue
holistically, including with existing Collection APIs.

• Is the problem being addressed significant enough to warrant a change to
Swift?

Yes. These are all common needs; when working with dictionaries, I find
myself writing `for` loops with `var`s far more often than I'd like.

• Does this proposal fit well with the feel and direction of Swift?

Yes, these are all pretty straightforward additions.

• If you have used other languages or libraries with a similar feature,
how do you feel that this proposal compares to those?

Ruby's `Hash` has many of these features and I appreciate them there;
`NSDictionary` does not and it suffers for it. Perl hashes have a
flattening behavior that tends to be amenable to editing them in various
ways, but that's not really suitable for Swift.

• How much effort did you put into your review? A glance, a quick reading,
or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Ricardo Parada) #15

Thanks Shawn.

I understand that part. I asked mostly because Nate mentioned that we cannot have a default closure because it would force the use of try even when the default closure does not throw. I do t know if that is a current bug or feature to allow changing the default in the future to a closure that throws:

https://bugs.swift.org/browse/SR-2979

Thanks.

···

On Apr 6, 2017, at 10:12 PM, Shawn Erickson <shawnce@gmail.com> wrote:

They rethrow which is IMHO perfect for this. If the closure throws then the outer functions throws (e.g. requires try). This allows folks that have a need to throw if they want without forcing it on everyone.

On Thu, Apr 6, 2017 at 6:53 PM Ricardo Parada via swift-evolution <swift-evolution@swift.org> wrote:

How important is it that the closure be allowed to throw when resolving collisions? In my use cases merging dictionaries he resolving of the collisions is always relatively simple. Shouldn’t they be as below maybe?

    /// Creates a new dictionary using the key/value pairs in the given sequence,
    /// using a combining closure to determine the value for any duplicate keys.
    init<S: Sequence>(
        merging keysAndValues: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element

    /// Merges the key/value pairs in the given sequence into the dictionary,
    /// using a combining closure to determine the value for any duplicate keys.
    mutating func merge<S: Sequence>(
        _ other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) where S.Iterator.Element == Element
    /// Returns a new dictionary created by merging the key/value pairs in the
    /// given sequence into the dictionary, using a combining closure to determine
    /// the value for any duplicate keys.
    func merged<S: Sequence>(
        with other: S,
        resolvingCollisionsWith combine: (Value, Value) -> Value
    ) -> [Key: Value] where S.Iterator.Element == Element

On Apr 6, 2017, at 12:51 PM, Nate Cook via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 9:43 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 5, 2017, at 5:45 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

  • What is your evaluation of the proposal?

(As a meta issue, I'm not sure I like the grab-bag review style; I'm finding this proposal a little bit difficult to navigate.)

Sequence-based initializer and merging initializer

Good idea, but I think these two are redundant with each other, and I don't think "merging" is an accurate way to describe what the second one does. (`merging` would suggest to me that it was combining several dictionaries or lists, not combining conflicting elements.) I'd suggest a single initializer along the lines of:

  init<S: Sequence>(_ keysAndValues: S, correctingConflictsBy resolveConflict: (Value, Value) throws -> Value = { fatalError("Duplicate values \($0) and \($1)") }) rethrows
    where S.Iterator.Element == (key: Key, value: Value)

Thanks for all your feedback, Brent! One note on this item in particular—if you specify a default argument for a throws/rethrows closure, you have to use "try" at the call site even if the default closure argument doesn't throw. Modules currently don't promise that default closure arguments don't throw, and a default argument could change from non-throwing to throwing in a later version of a library.

There's a bug tracking the issue here: https://bugs.swift.org/browse/SR-2979

Merging methods

Good idea, but I'm not a fan of the `mergingValues:` label. I would suggest the same `correctingConflictsBy resolveConflict:` label I suggested for the previous method—possibly including the default value. I also think `merge(_:correctingConflictsBy:)`'s first parameter should be labeled `with`, just as the `merged` variant is.

I wonder if we might also want a method that copies the Dictionary, but with a single key added/removed/changed:

  func withValue(_ value: Value?, forKey key: Key) -> [Key: Value]

Key-based subscript with default value

I like the functionality, but not way this proposal does it. I don't like having the default value be a labeled parameter to the subscript, because it isn't used to locate the value. However, I can't come up with a better syntax without adding language features. What I'd like to do is make it possible to assign through `??`:

  frequencies[c] ?? 0 += 1

But that would require either that we support `inout` functions, or that `??` become magic syntax instead of a standard library feature. The former is not coming in Swift 4 and the latter is less than ideal.

Still, if we would rather have that syntax and we think we'll soon have the language improvements needed to pull it off, I'd suggest rejecting this portion of the proposal.

Dictionary-specific map and filter

I am +114 on this. I say that because I have received 114 upvotes on my Stack Overflow answer explaining how to write a `Dictionary.map` method: <http://stackoverflow.com/questions/24116271/whats-the-cleanest-way-of-applying-map-to-a-dictionary-in-swift/24219069#24219069>

I agree with the decision not to pass keys to the closures in these methods; that keeps them simple and focused, and ensures they stay parallel with ordinary `map` and `filter`. I also agree with the decision to not build in a form of `map` which allows key remapping; you can always do that with the sequence-based initializer, which would let you add conflict-handling logic more elegantly than a key-value `map` could.

Visible dictionary capacity

I doubt I'll ever use the `capacity` property, but I'm not opposed to it. Adding a `reserveCapacity(_:)` method is a good idea.

A grouped(by:) method for sequences

Yes, please.

Apply relevant changes to Set

These make sense. (I considered suggesting the `filter` method be called `intersection(where:)`, but on second thought, I don't think that conveys the actual use cases for the method very well.)

I wonder if we might want to conform `Dictionary` to `SetAlgebra`. They have compatible semantics, and I've occasionally wanted to use them in the same places. On the other hand, some of the methods might form attractive nuisances; perhaps I should just write a SetAlgebra-conforming view when I want to do that.

General notes

If SE-0161 is accepted, we may want to support key path variants of some of these APIs (like `grouped(by:)`, `map(_:)`, and `filter(_:)`). On the other hand, we may want to defer that so we can consider that issue holistically, including with existing Collection APIs.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. These are all common needs; when working with dictionaries, I find myself writing `for` loops with `var`s far more often than I'd like.

  • Does this proposal fit well with the feel and direction of Swift?

Yes, these are all pretty straightforward additions.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Ruby's `Hash` has many of these features and I appreciate them there; `NSDictionary` does not and it suffers for it. Perl hashes have a flattening behavior that tends to be amenable to editing them in various ways, but that's not really suitable for Swift.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution