-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
[bugfix] isBetween should return false for invalid dates #4417
Conversation
@@ -226,7 +226,7 @@ test('is between year', function (assert) { | |||
assert.equal(m.isBetween( | |||
moment(new Date(2010, 5, 6, 7, 8, 9, 10)), | |||
moment(new Date(2011, 5, 6, 7, 8, 9, 10)), 'year'), false, 'year is later'); | |||
assert.equal(m.isBetween(m, 'year'), false, 'same moments are not between the same year'); | |||
assert.equal(m.isBetween(m, m, 'year'), false, 'same moments are not between the same year'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this.
assert.equal(+m, +mCopy, 'isBetween millisecond should not change moment'); | ||
}); | ||
|
||
test('is between invalid', function (assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a case to show that inclusivity flags don't affect this?
For example, #4416 was only an issue with the flags set to '[]'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, tests added for all inclusivity flag combos
@ashsearle thanks a lot! Very well done! |
Changes to
isBetween
to fix a couple of issues from #4416 and #4114 .Documentation states that
invalid.isBetween(another, another)
should returnfalse
, but #4416 highlights a case where it doesn't. This PR fixes #4416 and ensuresisBetween(..)
returnfalse
no matter what unit or inclusivity flags are used:The conversation in #4114 highlights some examples that are incompatible with current typescript bindings. This PR fixes the typescript binding to match example code - explicitly allowing
null
to be passed in as theunits
value, - and this now follows a faster code path to avoid cloning moments unnecessarily - i.e. fixes #4114.