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

Fix endOf bug when day light saving interferes with it #202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sazary
Copy link

@sazary sazary commented Mar 28, 2020

Description:

endOf has bug when it's used to get end of week for 2020-03-21 until 2020-03-27.

Steps To Reproduce:

Run this code snippet:

moment('2020-03-21 07:10:20').endOf('week')

Expected value: 2020-03-27
What it returns: 2020-03-28

Notes About Code

Please note that I've largely copied contents of endOf function from moment lib itself. Specifically, please see this switch.
I've tried to stick to code conventions of this repo, but if there's any problem please let me know.

@behrang
Copy link
Member

behrang commented Mar 30, 2020

I didn't put enough time to check this, however in the code, endOf is implemented in terms of startOf, which only checks for jmonth and jyear, and all other values are passed back to moment itself.

I checked some values:

moment('2020-03-19', 'YYYY-MM-DD').endOf('week').format() // 2020-03-21T23:59:59+04:30
moment('2020-03-20', 'YYYY-MM-DD').endOf('week').format() // 2020-03-21T23:59:59+04:30

moment('2020-03-21', 'YYYY-MM-DD').endOf('week').format() // 2020-03-22T00:59:59+04:30

moment('2020-03-22', 'YYYY-MM-DD').endOf('week').format() // 2020-03-28T23:59:59+04:30
moment('2020-03-23', 'YYYY-MM-DD').endOf('week').format() // 2020-03-28T23:59:59+04:30

The first two and the last two are correct by my calculations. However, the middle one is wrong, since that hour does not exist. Maybe we can fix that.

I don't get the reason for your code. Can you explain more?

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.

2 participants