Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create mapping functions for Dictionary #546

Merged
merged 2 commits into from
Aug 16, 2018
Merged

Create mapping functions for Dictionary #546

merged 2 commits into from
Aug 16, 2018

Conversation

guykogus
Copy link
Contributor

@guykogus guykogus commented Aug 15, 2018

🚀

These versions of map and compactMap return a Dictionary, rather than an Array.
I'm a bit unsure of the function names because of the clash with the built-in map and compactMap functions.


map seems to prioritise the new function. E.g. in

let intToString = [0: "0", 1: "1", 2: "2"]
let stringToInt = intToString.map { (key, value) in
    return (String(describing: key), Int(value)!)
}

stringToInt is of type [String: Int]. If you want it to return [(String, Int)], as the original function would do, you need to define the type explicitly. I.e.

let stringToInt: [(String, Int)] = intToString.map { (key, value) in

compactMap actually won't compile in a similar situation because it prioritises the built-in function. E.g.

let stringToInt = intToString.compactMap { (key, value) in
    guard let intValue = Int(value) else { return nil }
    return (String(describing: key), intValue)
}

will throw a compilation error because it thinks that the the closure passed to compactMap should contain a single argument, it won't accept a tuple. You need to explicitly define it as

let stringToInt: [String: Int] = intToString.compactMap { (key, value) in
// or
let stringToInt = intToString.compactMap { (key, value) -> (String, Int)? in

in order for it to compile and use the new function.

To use the original function you again need to explicitly define the type. I.e.

let words: [(String, IntWord)] = strings.compactMap { (key, value) in

What do you guys think, should we rename the functions? What names sound good to you?

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 4.
  • New extensions support iOS 8.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.
  • I have added a changelog entry describing my changes.

…a new `Dictionary`, rather than an `Array`.
@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Aug 15, 2018

4 Messages
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.
📖 iOS: Executed 523 tests, with 0 failures (0 unexpected) in 3.518 (4.162) seconds
📖 tvOS: Executed 492 tests, with 0 failures (0 unexpected) in 2.236 (2.636) seconds
📖 macOS: Executed 354 tests, with 0 failures (0 unexpected) in 1.439 (1.792) seconds

Generated by 🚫 Danger

@SD10
Copy link
Member

SD10 commented Aug 15, 2018

@guykogus This is interesting but I don't like how we're overloading map and compactMap with specific functionality here. The fact that the overload returns a specific type -- dictionary -- is counterintuitive to the general concept of map

@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #546 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage      94%   94.01%   +<.01%     
==========================================
  Files          68       68              
  Lines        2871     2875       +4     
==========================================
+ Hits         2699     2703       +4     
  Misses        172      172
Flag Coverage Δ
#ios 94.01% <100%> (ø) ⬆️
#osx 94.01% <100%> (+5.12%) ⬆️
#tvos 94.01% <100%> (+5.12%) ⬆️
Impacted Files Coverage Δ
.../Extensions/SwiftStdlib/DictionaryExtensions.swift 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb19497...c3ea829. Read the comment docs.

@guykogus
Copy link
Contributor Author

For me it makes more sense for map to actually return the same type, but you're right that overloading is not a good idea (especially considering the issues I outlined). So my question remains, what names sound good? Dictionary has a function called mapValues that does something similar, so maybe we could use mapKeysAndValues? Thoughts?

@SD10
Copy link
Member

SD10 commented Aug 15, 2018

@guykogus I'd be more accepting of mapKeysAndValues given your example of mapValues on Dictionary. It's just when I see something called map I expect it to be a polymorphic function

@guykogus
Copy link
Contributor Author

OK, updated the names.
I'm confused as to what you mean about map being polymorphic. Are these functions not polymorphic? They convert a Dictionary<K1, V1> into Dictionary<K2, V2>.

@guykogus guykogus changed the title Create map and compactMap functions for Dictionary Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Aug 15, 2018
@guykogus guykogus changed the title Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Aug 15, 2018
@guykogus guykogus changed the title Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Create _mapKeysAndValues_ and _compactMapKeysAndValues_ functions for Dictionary Aug 15, 2018
@guykogus guykogus changed the title Create _mapKeysAndValues_ and _compactMapKeysAndValues_ functions for Dictionary Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Aug 15, 2018
@guykogus guykogus changed the title Create mapKeysAndValues and compactMapKeysAndValues functions for Dictionary Create mapping functions for Dictionary Aug 15, 2018
@SD10
Copy link
Member

SD10 commented Aug 16, 2018

@guykogus I guess they are over the generic key/value, but the return type is still bound to Dictionary

Copy link
Member

@omaralbeik omaralbeik left a comment

Choose a reason for hiding this comment

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

@guykogus Interesting extension, and amazing that you added complexity as well 💯

@guykogus
Copy link
Contributor Author

@omaralbeik I stole the documentation from Apple's docs 😄. The complexity of compactMap is declared to be O(m + n), so for the dictionary it would probably be more like O(m + (2 * n)), but since constants are ignored for big-O notation it's still technically correct.

@guykogus guykogus merged commit 9eb6259 into master Aug 16, 2018
@guykogus guykogus deleted the dictionary-map branch August 16, 2018 06:47
@LucianoPAlmeida
Copy link
Member

Sorry couldn't be on the discussion 😭
But cool extensions @guykogus :))

@guykogus
Copy link
Contributor Author

I'm a big fan, especially for using dictionaries where the keys are enums that I eventually need to pass as the parameters to an HTTP call, so they need to be converted to their raw values. I feel like there'd be lots of useful scenarios for this...

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