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(#60): fix holiday matchers when date is not start of day #61

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

att14
Copy link
Contributor

@att14 att14 commented Dec 1, 2021

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #61 (8f6a33c) into master (b870dcf) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #61   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          116       128   +12     
  Branches        23        29    +6     
=========================================
+ Hits           116       128   +12     
Impacted Files Coverage Δ
src/holidays.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b870dcf...8f6a33c. Read the comment docs.

@amaidah
Copy link
Owner

amaidah commented Dec 10, 2021

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
Copy link
Owner

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({
Copy link
Owner

@amaidah amaidah Feb 17, 2022

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?

Copy link
Owner

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.

Copy link
Owner

Choose a reason for hiding this comment

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

#65

Copy link
Owner

@amaidah amaidah left a 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

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.

3 participants