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

Add random and randomBetween extensions to Date #326 #336

Merged
merged 4 commits into from
Dec 14, 2017

Conversation

akuzminskyi
Copy link
Member

Add random extensions to Date #326

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.

@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Dec 9, 2017

1 Warning
⚠️ Consider adding tests for new extensions or updating existing tests for a modified SwifterSwift extension
1 Message
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Dec 9, 2017

Codecov Report

Merging #336 into master will increase coverage by 0.01%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage   89.64%   89.65%   +0.01%     
==========================================
  Files          55       55              
  Lines        3323     3336      +13     
  Branches      154      155       +1     
==========================================
+ Hits         2979     2991      +12     
  Misses        338      338              
- Partials        6        7       +1
Flag Coverage Δ
#ios 89.8% <92.3%> (+0.01%) ⬆️
#osx 89.84% <92.3%> (ø) ⬆️
#tvos 89.8% <92.3%> (+0.01%) ⬆️
Impacted Files Coverage Δ
Sources/Extensions/Foundation/DateExtensions.swift 99.79% <92.3%> (-0.21%) ⬇️

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 25e1efc...140d378. Read the comment docs.

@omaralbeik omaralbeik requested a review from SD10 December 10, 2017 18:05
/// - startDate: minimum date (default is Date.distantPast)
/// - endDate: maximum date (default is Date.distantFuture)
/// - Returns: random date between two dates.
public static func random(since startDate: Date = Date.distantPast,
Copy link
Member

Choose a reason for hiding this comment

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

Hey @akuzminskyi, thanks for submitting this PR! 😃
What do you think about changing the external parameter names?
since to from
to to upTo

Copy link
Member Author

Choose a reason for hiding this comment

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

English is not my native language, so if you think that will be better I'll change it.

@@ -860,4 +860,58 @@ final class DateExtensionsTests: XCTestCase {
XCTAssertEqual(date, dateFromUnixTimestamp)
}

func testRandom() {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could just combine all the tests below into this single test.
I would like to maintain a 1 : 1 ratio between tests and extensions. This way when they fail we are clear of the cause.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, thanks. I'll do it

Combined tests for Date.radom into single test
CHANGELOG.md Outdated
@@ -25,7 +25,9 @@ N/A
### Enhancements
- New **NSImage** extensions:
- added `write(to url: URL, fileType type: _, compressionFactor: _)` to write NSImage to url. [#320](https://github.com/SwifterSwift/SwifterSwift/pulls/320) by [omaralbeik](https://github.com/omaralbeik).

- New **Date** extensions
- added `random(since startDate: Date = Date.distantPast, to endDate: Date = Date.distantFuture) -> Date` method that return radom date in in the specified range [#336](https://github.com/SwifterSwift/SwifterSwift/pull/336) by [akuzminskyi](https://github.com/akuzminskyi).
Copy link
Member

Choose a reason for hiding this comment

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

We've changed this now:
random(from:upTo:) -> Date will be fine for CHANGELOG entry. You don't need the full signature. There's also a CHANGELOG merge conflict we need to resolve

@SD10 SD10 merged commit 7a1b24c into SwifterSwift:master Dec 14, 2017
@SD10
Copy link
Member

SD10 commented Dec 14, 2017

Thank you for your contribution @akuzminskyi 💯 👍

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.

3 participants