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 isBetween method to date extension #248

Merged
merged 4 commits into from Sep 16, 2017
Merged

Add isBetween method to date extension #248

merged 4 commits into from Sep 16, 2017

Conversation

be-meyer
Copy link
Member

Added a isBetween method to the date extension which can be used to determine if a date is between two given dates including or excluding the given dates itself. Default is to include the dates itself.

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 3.
  • 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.

@omaralbeik omaralbeik self-requested a review September 16, 2017 16:57
@omaralbeik omaralbeik self-assigned this Sep 16, 2017
/// - Parameter endDate: endDate date to compare self to.
/// - Parameter includeBounds: true if the start and end date should be included (default is true)
/// - Returns: true if the date is between the two given dates.
public func isBetween(_ startDate: Date, _ endDate: Date, includeBounds: Bool = true) -> Bool {
Copy link
Member

@SD10 SD10 Sep 16, 2017

Choose a reason for hiding this comment

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

I'm fine with this. Clean & well documented 🎉 I'm just curious if we don't need the extension and can use:

let date1 = Date()
let date2 = Date()
let date3 = Date()
let bool = (date1...date2).contains(date3)

I agree having the bounds check makes this easier though. I would like to see it default to false though. Because between is technically not inclusive. What do you think @Bennx?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SD10 Of cause it does make more sense to have it set to false.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also like the Range idea. Lets see if i can figure that out.

Copy link
Member

Choose a reason for hiding this comment

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

@Bennx The implementation doesn't need to be changed. It's fine as is. This could be more performant actually.

Copy link
Member

Choose a reason for hiding this comment

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

@Bennx No, we shouldn't add that extension. This is already possible and would conflict with the Swift standard library.

@codecov
Copy link

codecov bot commented Sep 16, 2017

Codecov Report

Merging #248 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
+ Coverage   93.63%   93.66%   +0.03%     
==========================================
  Files          97       97              
  Lines        5544     5572      +28     
==========================================
+ Hits         5191     5219      +28     
  Misses        353      353
Impacted Files Coverage Δ
Tests/FoundationTests/DateExtensionsTests.swift 94.92% <100%> (+0.2%) ⬆️
Sources/Extensions/Foundation/DateExtensions.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 79cb9f1...f3b57a2. Read the comment docs.

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.

@Bennx Can you add a CHANGELOG entry for this that follows the same format we are using?

@be-meyer
Copy link
Member Author

@SD10 hope the format fits.

@SD10
Copy link
Member

SD10 commented Sep 16, 2017

@Bennx This would go under the Next Release section. v3.2.1 would be a bug fix release. We always add new CHANGELOG entries to the Next Release section

@be-meyer
Copy link
Member Author

@SD10 Got it. Cheers!

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.

Thanks @Bennx for seeing this from start to finish

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.

Thank you @Bennx @SD10 🎉

@SD10 SD10 merged commit f52e8dd into SwifterSwift:master Sep 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants