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 issue #1637: setDay with weekStartsOn != 0 #1639

Merged
merged 4 commits into from
Apr 9, 2020

Conversation

oakify
Copy link

@oakify oakify commented Feb 14, 2020

As demonstrated in the issue, setDay(date, day, [option]) returns an
incorrect value if date is on a Sunday and option.weekStartsOn is != 0.
This includes a fix (there's probably a nicer way to code this) and
2 corresponding test cases.

I also found 4 test cases that I believe to be incorrect, all using
the argument {weekStartsOn: 1}. I hope I'm not mistaken here, but I
changed the test cases accordingly.

  • All tests completed

  • Build script run

  • Added entry to Unreleased section of CHANGELOG

src/setDay/test.js Outdated Show resolved Hide resolved
@dcousens
Copy link
Contributor

dcousens commented Feb 14, 2020

@boomdev please see my suggested update to the examples in oakify#1

@trollepierre
Copy link

🙏 @dcousens : can you approve this PR since the change has been taken into consideration?

  • I opened a new bug that count on this fix

@trollepierre
Copy link

@boomdev : have you checked if "lastDayOfWeek startOfWeek differenceInCalendarWeeks" are fixed?

@dcousens
Copy link
Contributor

@trollepierre I am not a maintainer, I am merely a fellow user wanting to help 👍

@oakify
Copy link
Author

oakify commented Mar 30, 2020

@boomdev : have you checked if "lastDayOfWeek startOfWeek differenceInCalendarWeeks" are fixed?

@trollepierre I have not.

@dkozickis
Copy link
Contributor

Thank you @boomdev and @dcousens, this seems GTG.
Tests might be failing on CI because of DST/Timezones, not sure @kossnocorp

Xavier Tourenq added 3 commits April 9, 2020 19:26
As demonstrated in the issue, setDay(date, day, [option]) returns an
incorrect value if date is on a Sunday and option.weekStartsOn is != 0.
This includes a fix (there's probably a nicer way to code this) and
2 corresponding test cases.

I also found 4 test cases that I believe to be incorrect, all using
the argument {weekStartsOn: 1}. I hope I'm not mistaken here, but I
changed the test cases accordingly.

- All tests completed

- Build script run

- Added entry to Unreleased section of CHANGELOG
@kossnocorp
Copy link
Member

@dkozickis yeah, some tests were failing because of a DST issue that was fixed in master. I've rebased the branch but there's still a couple failing tests in the changed function. I'm on it.

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.

5 participants