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

New dictionary extensions. #193

Merged
merged 1 commit into from
Jul 12, 2017
Merged

New dictionary extensions. #193

merged 1 commit into from
Jul 12, 2017

Conversation

LucianoPAlmeida
Copy link
Member

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • I have only one commit in this pull request.
  • New extensions support iOS 8 or later.
  • New extensions are written in Swift 3.
  • I have added tests for new extensions, and they passed.
  • Pull request was created to master head branch.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All comments headers have the word SwifterSwift: before description.

/// SwifterSwift: Return every key of the dictionary as a tuple array. There's no order guaranteed.
///
/// - Returns: key/value of dictionary as a tuple array.
public var tuples : [(key: Key, value: Value)] {
Copy link
Member

Choose a reason for hiding this comment

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

We really shouldn't use computed properties for functionality with complexity > O(1)

@codecov
Copy link

codecov bot commented Jul 12, 2017

Codecov Report

Merging #193 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage    92.8%   92.82%   +0.01%     
==========================================
  Files          85       85              
  Lines        4756     4767      +11     
==========================================
+ Hits         4414     4425      +11     
  Misses        342      342
Impacted Files Coverage Δ
Source/Extensions/DictionaryExtensions.swift 100% <100%> (ø) ⬆️
.../SwifterSwiftTests/DictionaryExtensionsTests.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 4d8b379...3fc783e. Read the comment docs.

/// SwifterSwift: Return every key of the dictionary as a tuple array. There's no order guaranteed.
///
/// - Returns: key/value of dictionary as a tuple array.
public func tuples() -> [(Key,Value)] {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this extension is not a good fit for SwifterSwift.
It specializes the generic abstraction created by map -- It is just map { $0 }

///
/// - Parameter where: condition to evaluate each tuple entry against.
/// - Returns: Filtered key/value of dictionary as a tuple array.
public func tuples(where condition: @escaping ((key: Key, value: Value)) throws -> Bool) rethrows -> [(Key, Value)] {
Copy link
Member

@SD10 SD10 Jul 12, 2017

Choose a reason for hiding this comment

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

Same as above. This is just a wrapper around flatMap. There isn't much to gain here. Whether you use flatMap or tuples you will have to write out the closure. This function is just constricting our return type, essentially, a type constrained flatMap.

///
/// - Parameter where: condition to evaluate each tuple entry against.
/// - Returns: Count of entries that matches the where clousure.
public func count(where condition: @escaping ((key: Key, value: Value)) throws -> Bool ) rethrows -> Int {
Copy link
Member

Choose a reason for hiding this comment

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

Looks great 👍

Copy link
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

See comments above

@LucianoPAlmeida LucianoPAlmeida merged commit f371fcf into SwifterSwift:master Jul 12, 2017
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.

2 participants