Skip to content

Add sequence-based initializers and merge methods to Dictionary#125

Merged
dabrahams merged 6 commits into
swiftlang:masterfrom
natecook1000:natecook-dictionary-merge
May 26, 2016
Merged

Add sequence-based initializers and merge methods to Dictionary#125
dabrahams merged 6 commits into
swiftlang:masterfrom
natecook1000:natecook-dictionary-merge

Conversation

@natecook1000

Copy link
Copy Markdown
Member

The Dictionary type should allow initialization from a sequence of (Key, Value) tuples and offer methods that merge a sequence of (Key, Value) tuples with an existing dictionary. Each of these new APIs would optionally take a closure to combine values for duplicate keys.

View Proposal →

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Cumbersome and unfriendly" is a subjective judgement, unfortunately. One can also say that checking for key uniqueness is "preventing errors" and "is making code more explicit". I think both points are valid, but we need more data to understand how much convenience we are getting with implicit behavior that drops the keys vs. how many bugs we would be hiding that way. Please keep in mind that these could easily be security bugs, if one, say, uses dictionary to handle HTTP headers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. One solution could be to make the initializer/methods without the combine closure parameter failable or marked throws. A caller would then have to reckon with the possibility of duplicate keys one of three ways:

  1. Let the initializer check for duplicate keys and accept a nil return if duplicates exist,
  2. Unwrap the resulting optional dictionary with !—this could be used if they check first or have static knowledge that the sequence doesn't have any duplicate keys, or
  3. Provide a closure to handle any cases with duplicate keys.

Someone wanting a Dictionary back after filtering a dictionary on the value could use the second option, since the intermediate sequence would have a subset of the original dictionary's keys. I'm not sure of a case where someone would want the first option, which would likely push people toward the other two options.

Another solution could be to always require the closure, and in the documentation provide { (old, _) in old } and { (_, new) in new } as possible examples for keeping the first and last values, respectively.

Interestingly enough, NSDictionary opts for the exact opposite behavior as this proposal, silently keeping the first value given for a key when created via dictionaryWithObjects:forKeys::

NSDictionary(objects: [1, 2, 3, 4], forKeys: ["a", "b", "c", "a"])
// ["b": 2, "c": 3, "a": 1]

While NSMutableDictionary aligns with this proposal, silently keeping the last value!

NSMutableDictionary(objects: [1, 2, 3, 4], forKeys: ["a", "b", "c", "a"])
// ["b": 2, "c": 3, "a": 4]

So, I suppose it would be nice to have something consistent for Dictionary in Swift if this proposal is accepted.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also want to note that having the ! in users code helps a lot with pointing out the possible trap.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this is what I've been using so far:

extension Dictionary {
    /** 
     Creates a dictionary from an array of key-value pairs.
     Will assert if a key is repeated and !ignoringDuplicateKeys.
     */
    public init(elements: [(Key, Value)], ignoringDuplicateKeys: Bool = false) {

I agree that not ignoring duplicates by default is probably the safest. The problem with returning nil in the case of found duplicates is that there would be no way to ignore them other than ensuring that the input is free of duplicates.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm modifying the proposal to convert the simple init to be a failable init? and the merging init to use a marging label for the sequence: init(merging:combine:). The new APIs under the revised proposal would be:

init?<S: SequenceType where S.Generator.Element == (Key, Value)>(
    _ sequence: S)

init<S: SequenceType where S.Generator.Element == (Key, Value)>(
    merging sequence: S, 
    @noescape combine: (Value, Value) throws -> Value
    ) rethrows
mutating func mergeContentsOf<S: SequenceType where S.Generator.Element == (Key, Value)>(
    _ sequence: S, 
    @noescape combine: (Value, Value) throws -> Value
    ) rethrows
func mergedWith<S: SequenceType where S.Generator.Element == (Key, Value)>(
    _ sequence: S, 
    @noescape combine: (Value, Value) throws -> Value) rethrows -> [Key: Value]

@gribozavr: I think this should address your concerns and let the caller decide what level of error checking they want. Thanks again for your comments, they really helped me clarify the difference between the two kinds of functionality requested here.
@NachoSoto: With this revision you would use the merging init to ignore duplicate keys and choose specifically which value to take. How does this look?

let dupes = [("a", 1), ("b", 2), ("a", 3), ("b", 4)]
let firstValueDictionary = Dictionary(merging: dupes) { (first, _) in first }
// ["b": 2, "a": 1]
let lastValueDictionary = Dictionary(merging: dupes) { (_, last) in last }
// ["b": 4, "a": 3]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is perfect 👍

@dabrahams dabrahams self-assigned this Feb 9, 2016
@natecook1000 natecook1000 force-pushed the natecook-dictionary-merge branch from 8af5ddb to b28bd4c Compare March 22, 2016 06:09
@natecook1000

Copy link
Copy Markdown
Member Author

@dabrahams Updated for Swift 3 APIs—any other info I can provide on this proposal?

@lattner

lattner commented Apr 28, 2016

Copy link
Copy Markdown
Contributor

@dabrahams What's the status on this, does this make sense to merge? If so, please do and link it into schedule.md as an unscheduled proposal. Thanks.

@dabrahams dabrahams merged commit 9479fef into swiftlang:master May 26, 2016
@natecook1000 natecook1000 deleted the natecook-dictionary-merge branch April 22, 2022 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants