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

[bugfix] Calculate daysInMonth without Date(), fixes #3358 #3875

Closed
wants to merge 3 commits into from

Conversation

marwahaha
Copy link
Member

fixes #3358

@maggiepint
Copy link
Member

I think this works (and if it does, it's very nice) but can we explicitly test the overflow/underflow case that Matt describes in the issue?
controversial opinion to follow
I wouldn't rely on the public API of moment to test this. Explicitly test THAT function - just to document how the function works.

@butterflyhug
Copy link
Contributor

I'm not sure if this actually matters for any of the ways we actually use this internal function, but it's worth noting:

By removing Date's overflow behavior when months is < 0 or >= 12, I'm pretty sure you are changing the previous behavior of this function. To be clear, I don't mind the new behavior if the change doesn't break anything, it's just that we should double-check that.

@mattgrande
Copy link
Contributor

(31 - month % 7 % 2)

^ It took me awhile to figure out what was going on here, but this is brilliant.

@marwahaha
Copy link
Member Author

Ok, I added some tests detailing the overflow behavior.

if (modMonth === month) {
return month === 1 ? (isLeapYear(year) ? 29 : 28) : (31 - month % 7 % 2);
}
return daysInMonth(year + (month - modMonth) / 12, modMonth);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we please update year if we have a modMonth and not use recursion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it in 475d4c1

@ichernev ichernev changed the title Calculate daysInMonth without Date() [bugfix] Calculate daysInMonth without Date(), fixes #3358 Aug 3, 2017
@ichernev
Copy link
Contributor

ichernev commented Aug 6, 2017

Merged in f5953bd

@ichernev ichernev closed this Aug 6, 2017
ichernev added a commit that referenced this pull request Aug 6, 2017
ichernev added a commit that referenced this pull request Aug 6, 2017
[bugfix] Calculate daysInMonth without Date(), fixes #3358
fbonzon pushed a commit to fbonzon/moment that referenced this pull request Oct 28, 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.

Feb 29 0000 is invalid
5 participants