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: Partial holidays range breaks #2297

Merged

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Nov 18, 2024

  • Partial holidays were not generating valid range breaks, as they did not account for business periods specified on the calendar
  • Needed for DH-16016
  • Added a bunch of test cases

- Partial holidays were not generating valid range breaks, as they did not account for business periods specified on the calendar
- Added a bunch of test cases
@mofojed mofojed requested a review from a team November 18, 2024 20:16
@mofojed mofojed self-assigned this Nov 18, 2024
@mofojed mofojed requested review from vbabich and removed request for a team November 18, 2024 20:16
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 10 lines in your changes missing coverage. Please review.

Project coverage is 46.68%. Comparing base (5ebc195) to head (8ed856c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/chart/src/ChartUtils.ts 88.88% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
+ Coverage   46.60%   46.68%   +0.07%     
==========================================
  Files         704      704              
  Lines       38963    39011      +48     
  Branches     9914     9926      +12     
==========================================
+ Hits        18158    18211      +53     
+ Misses      20794    20746      -48     
- Partials       11       54      +43     
Flag Coverage Δ
unit 46.68% <88.88%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for (let i = 0; i < weekLength; i += 1) {
if (
!businessDaysSet.has(i) &&
businessDaysSet.has((i - 1) % weekLength)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work when businessDaysSet has all days except 0?
(i - 1) % weekLength is out of bounds for i = 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh great catch! No it does not, I'll fix up.

packages/chart/src/ChartUtils.ts Outdated Show resolved Hide resolved
mofojed and others added 3 commits November 27, 2024 09:38
Co-authored-by: Vlad Babich <vladimir.babich@gmail.com>
- Had an off by 1 error when calculating the business days
@mofojed mofojed requested a review from vbabich November 27, 2024 14:45
@mofojed mofojed merged commit ab4c02f into deephaven:main Nov 27, 2024
11 checks passed
@mofojed mofojed deleted the bender_DH-16016-business-time-holiday-overlap branch November 27, 2024 18:08
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants