-
Notifications
You must be signed in to change notification settings - Fork 13
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(#60): fix holiday matchers when date is not start of day #61
Conversation
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 116 128 +12
Branches 23 29 +6
=========================================
+ Hits 116 128 +12
Continue to review full report at Codecov.
|
Thank you, will take a look this weekend |
@@ -92,11 +101,14 @@ export const isLaborDay = function(inst) { | |||
}); | |||
const weekday = firstDayInSeptember.weekday; | |||
const isFirstDayLaborDay = weekday === 1; | |||
const memorialDay = isFirstDayLaborDay | |||
const laborDay = isFirstDayLaborDay |
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 spotting this typo
const nyd2016 = DateTime.fromObject({ year: 2016, month: 1, day: 1 }); | ||
const nyd1988 = DateTime.fromObject({ year: 1988, month: 1, day: 1 }); | ||
const nyd2022 = DateTime.fromObject({ year: 2022, month: 1, day: 1 }); | ||
const nyd2019 = DateTime.fromObject({ |
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.
Could we keep some tests that don't have hours explicitly defined?
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.
Actually, a better path forward now that I think about it more would be to create a series of tests for the various different ways to create a DateTime
so thats .local()
.fromObject
.fromISO
.now()
etc.
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.
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.
Awesome, thanks for spotting and fixing this -- this should be good to merge if we add a few tests back that don't have explicit hours. I think a section of tests that verify the methods work both with and without explicit hours is ideal
No description provided.