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

Fix inconsistent moment.min and moment.max results #2438

Closed
wants to merge 5 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@cwohlman
Contributor

cwohlman commented Jun 26, 2015

Fixes #2362, includes updated tests for moment.min and moment.max.

Also includes tests to show that moment.isBefore and moment.isAfter correctly handle invalid moments.

cwohlman added some commits Jun 26, 2015

Ensure moment.isAfter and moment.isBefore match js math operations
Add tests to isBefore and isAfter preparatory to fixing #2362, since moment.min and moment.max rely on isBefore and isAfter to calculate moment value
@wyantb

This comment has been minimized.

Contributor

wyantb commented Jun 27, 2015

While you're in the neighborhood, would it make sense to treat isSame in a similar manner, per #2354?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 27, 2015

@cwohlman you said you'd filter out invalid moments in the issue but now you return invalid If any one of them is invalid. Also you can change isSame isAfter and isBefore to return false if they hit invalid.

@cwohlman

This comment has been minimized.

Contributor

cwohlman commented Jun 27, 2015

Actually, as suggested here: #2362 (comment), I wrote the fix to match the Math.min and Math.max behavior. Also, I noticed that isBefore isAfter match the Math operations < and >

Although in my original use case I would have preferred for the invalid moments to be filtered out, I think this implementation makes more sense on the general principle that math operations involving NaN typically return NaN. 

@jisaacks

This comment has been minimized.

Contributor

jisaacks commented Jun 27, 2015

I think this implementation makes more sense on the general principle that math operations involving NaN typically return NaN.

I think that makes sense, the user can always filter out invalid moments before getting the min/max.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jun 28, 2015

So you only added tests for before/after methods -- so they just worked? Also can you please add isSame to the bunch?

I agree that the filtering can be left out.

@ichernev ichernev added this to the 2.10.5 milestone Jun 28, 2015

@cwohlman

This comment has been minimized.

Contributor

cwohlman commented Jun 29, 2015

I'd be happy to add isSame to this pull request, don't have time right this minute, but I'll try to get it in sometime today.

valueOf should return NaN for any invalid moment
this fixes an issue where isSame would return true for two invalid moments, which is inconsistent with NaN === NaN and also inconsistent with other cases where isSame would return false for two invalid moments. (some invalid moments are backed by a valid date)

Fixes #2354
@cwohlman

This comment has been minimized.

Contributor

cwohlman commented Jun 29, 2015

Note that my latest changes modify the behavior of moment.valueOf in cases where an invalid moment is backed by a valid date.

@cwohlman

This comment has been minimized.

Contributor

cwohlman commented Jun 29, 2015

@ichernev correct, the before and after methods already exhibited the correct behavior.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2015

@cwohlman would this "edge" case (invalid moment backed by valid date) happen if you create an invalid moment, but then modify it, in the current code? If yes, I'd say a lot of people "depend" on this functionality, i.e. we'll break a lot of code if we start propagating invalid moments. Also if there is valid date attached, all of the moment methods will just work correctly, and it will be almost unnoticeable (that the moment is invalid).

@cwohlman

This comment has been minimized.

Contributor

cwohlman commented Jul 4, 2015

@ichernev correct. Let's move further discussion of the issue to #2354, and I'll update my pull request without the edge-case.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 4, 2015

@cwohlman thank you! Will merge in next release.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 13, 2015

Merged in c410da4

@ichernev ichernev closed this Jul 13, 2015

ichernev added a commit that referenced this pull request Jul 13, 2015

Merge pull request #2438 from cwohlman:fixInvalidMin
Fix inconsistent moment.min and moment.max results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment