Add businessDiff method #45

kristremblay · 2021-02-17T23:15:48Z

Added businessDiff method to get number of business days between DateTimes.

After completing the switch from Moment to Luxon, I realized this functionality was missing in this library. The solution was to borrow from https://github.com/kalmecak/moment-business-days/blob/master/index.js.

Usage:

const targetDate = DateTime.local().plusBusiness({ days: 10 });

const businessDaysDiff = DateTime.local().businessDiff(targetDate);

// businessDaysDiff = 10

between DateTimes.

In practice this would be handled based on developer needs.

codecov-io · 2021-02-20T19:59:48Z

Codecov Report

Merging #45 (6a81a34) into master (3ca6291) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #45   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          116       133   +17     
  Branches        23        33   +10     
=========================================
+ Hits           116       133   +17

Impacted Files	Coverage Δ
src/index.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 3ca6291...6a81a34. Read the comment docs.

kristremblay · 2021-02-20T20:08:40Z

@amaidah oddly, the tests were passing when this was submitted but when I opened vscode today they were failing. I've refactored to clear out some of the spider webs left over from a day of working :)

amaidah · 2021-02-27T22:43:10Z

Hey, thanks for opening a PR -- I will take a look (can't promise this weekend). I remember thinking about this method originally and had thought about changing the interface a bit.

kristremblay · 2021-02-28T17:40:51Z

Sounds good, and you're welcome 😄

added includeEndDate

kristremblay · 2021-03-03T18:12:27Z

Swapped out the relative param for a config object.

kristremblay · 2021-04-23T18:30:24Z

@amaidah sorry to bother, but have you had a chance to take a peek?

amaidah · 2021-04-26T17:50:59Z

@amaidah sorry to bother, but have you had a chance to take a peek?

Thanks for being super patient, not yet. Will try for this week

src/index.test.js

src/index.js

amaidah · 2021-04-29T06:26:42Z

src/index.js

+/**
+ * @typedef BusinessDiffConfig
+ * @property {boolean} [includeEndDate=false] - include the end date in the calculation
+ * @property {boolean} [relative=false] - signs the return value as negative if end date is in the past


If I think about how I use a calendar, I say things like 2 days ago (-2) so it doesn't make total sense to me that a difference would be absolute by default. Is there a reason for this? I remember being confused with the moment version of this plugin when I first saw that too.

Both diff and diffNow are relative and I'd like to to keep this plugin similar to Luxon core concepts

I think the rationale is that you already have methods for determining whether a date is in the past and it is probably a less common scenario to need a negative value for a business day calculation.

That being said, consistency is a really strong argument so if you think it would be better defaulting relative to true I will support that.

Ok cool, let's go with relative as default

src/index.js

amaidah · 2021-04-29T06:47:57Z

src/index.test.js

+
+  it('knows there are 3 business days between two dates that are 3 business days apart', () => {
+    const myCompanyTakesNoHolidays = [];
+    const startDate = DateTime.local().startOf('day');


is .startOf necessary for the tests?

In all honestly, it's been a while and it's possible that startOf only seemed necessary as a symptom of another resolved problem.

Removing them on local seems to not affect the tests, perhaps we can refactor them out.

Co-authored-by: Andrew Maidah <[email protected]>

codecov-commenter · 2021-04-29T15:34:20Z

Codecov Report

Merging #45 (944ebb5) into master (3ca6291) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #45   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          116       136   +20     
  Branches        23        34   +11     
=========================================
+ Hits           116       136   +20

Impacted Files	Coverage Δ
src/index.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 3ca6291...944ebb5. Read the comment docs.

src/index.test.js

Put startOf call back in. Co-authored-by: Andrew Maidah <[email protected]>

kristremblay · 2021-05-01T20:38:01Z

Hmm...running into some failing tests when the current date is not a valid business day. Going to investigate.

How should calculations work if performed on a non-business day?

kristremblay · 2021-05-01T21:14:12Z

Ok upon further investigation, it was already working correctly, it was just a matter of a bad test.

I am interested in your input on this, but maybe we should have a separate test for starting on a non-business day.

amaidah · 2021-05-01T21:40:29Z

How should calculations work if performed on a non-business day?

Should just return the number of days that are business days between the two compared dates right? So if starting on Sat, and the next day is Sun and both are nonbusiness running a diffBusiness on that range should return 0. I'm curious what would happen though if includeEndDate were true in this scenario

I am interested in your input on this, but maybe we should have a separate test for starting on a non-business day.

Yes, now that you bring it up, sounds smart to test for a few more things:

Starts on a non-business day (in general)
Ends on non-business day (in general)
Starts on business day but ends on non-business day and includes end date
Start on non-business day and ends on non-business day with no business days in the range and includes end date

I also include error handling conditional for the other methods:

  if (!dt.isValid) {
    return dt;
  }

Maybe we should think about returning an error if trying to call diffBusiness on something that isn't a DateTime

kristremblay · 2021-05-01T21:48:43Z

I'm curious what would happen though if includeEndDate were true in this scenario

That's actually a good point that isn't currently accounted for. I just added the following:

let daysDiff = Number(
  includeEndDate && end.isBusinessDay() && !end.isHoliday()
);

Testing for starting on a non-business day should be next on the list. I will work on that over the weekend. In the meantime, I've updated the branch to use a static start date of Monday May 3 2021.

amaidah · 2021-05-10T23:59:18Z

I'm curious what would happen though if includeEndDate were true in this scenario

That's actually a good point that isn't currently accounted for. I just added the following:
let daysDiff = Number(
  includeEndDate && end.isBusinessDay() && !end.isHoliday()
);
Testing for starting on a non-business day should be next on the list. I will work on that over the weekend. In the meantime, I've updated the branch to use a static start date of Monday May 3 2021.

FYI, I merged in a bunch of dependabot PRs so you may need to pull from master

kristremblay · 2021-05-11T19:41:42Z

Thanks for letting me know!

hdoan-codaio · 2022-02-11T01:14:33Z

I'm interested in using this method. Can you merge the PR?

kristremblay added 3 commits February 17, 2021 17:10

Added businessDiff method to get number of buiness days

561d128

between DateTimes.

Fixed failing test due to time issue.

f49c59b

In practice this would be handled based on developer needs.

Clean tests, fixed bug in while to incl. last day.

11c3b05

Reverted < to <= in day count loop.

6a81a34

Replaced relative param with config object

34e8226

added includeEndDate

kristremblay added 3 commits March 3, 2021 12:14

Fix break in documentation.

f04f736

Update businessDiff description.

092114f

Improved BusinessDiffConfig description

bddb103

amaidah reviewed Apr 29, 2021

View reviewed changes

kristremblay and others added 4 commits April 29, 2021 09:08

Update src/index.test.js

37e7a5a

Co-authored-by: Andrew Maidah <[email protected]>

Update src/index.js

0b8bf9c

Co-authored-by: Andrew Maidah <[email protected]>

Rename businessDiff diffBusiness for consistency

bfd9b84

Readme regen

e4f92b0

Removed start of day from tests

a26325e

amaidah reviewed May 1, 2021

View reviewed changes

src/index.test.js Outdated Show resolved Hide resolved

amaidah reviewed May 1, 2021

View reviewed changes

src/index.test.js Outdated Show resolved Hide resolved

Update src/index.test.js

c2288c1

Put startOf call back in. Co-authored-by: Andrew Maidah <[email protected]>

Test cleanup and includeEndDate fix

2f2a411

Fixed missing coverage

944ebb5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add businessDiff method #45

Add businessDiff method #45

kristremblay commented Feb 17, 2021 •

edited

Loading

codecov-io commented Feb 20, 2021 •

edited

Loading

kristremblay commented Feb 20, 2021

amaidah commented Feb 27, 2021

kristremblay commented Feb 28, 2021

kristremblay commented Mar 3, 2021

kristremblay commented Apr 23, 2021

amaidah commented Apr 26, 2021

amaidah Apr 29, 2021

kristremblay Apr 29, 2021

amaidah May 1, 2021

amaidah Apr 29, 2021

kristremblay Apr 29, 2021

kristremblay Apr 29, 2021

codecov-commenter commented Apr 29, 2021 •

edited

Loading

kristremblay commented May 1, 2021 •

edited

Loading

kristremblay commented May 1, 2021

amaidah commented May 1, 2021 •

edited

Loading

kristremblay commented May 1, 2021

amaidah commented May 10, 2021

kristremblay commented May 11, 2021

hdoan-codaio commented Feb 11, 2022

Add businessDiff method #45

Are you sure you want to change the base?

Add businessDiff method #45

Conversation

kristremblay commented Feb 17, 2021 • edited Loading

codecov-io commented Feb 20, 2021 • edited Loading

Codecov Report

kristremblay commented Feb 20, 2021

amaidah commented Feb 27, 2021

kristremblay commented Feb 28, 2021

kristremblay commented Mar 3, 2021

kristremblay commented Apr 23, 2021

amaidah commented Apr 26, 2021

amaidah Apr 29, 2021

Choose a reason for hiding this comment

kristremblay Apr 29, 2021

Choose a reason for hiding this comment

amaidah May 1, 2021

Choose a reason for hiding this comment

amaidah Apr 29, 2021

Choose a reason for hiding this comment

kristremblay Apr 29, 2021

Choose a reason for hiding this comment

kristremblay Apr 29, 2021

Choose a reason for hiding this comment

codecov-commenter commented Apr 29, 2021 • edited Loading

Codecov Report

kristremblay commented May 1, 2021 • edited Loading

kristremblay commented May 1, 2021

amaidah commented May 1, 2021 • edited Loading

kristremblay commented May 1, 2021

amaidah commented May 10, 2021

kristremblay commented May 11, 2021

hdoan-codaio commented Feb 11, 2022

kristremblay commented Feb 17, 2021 •

edited

Loading

codecov-io commented Feb 20, 2021 •

edited

Loading

codecov-commenter commented Apr 29, 2021 •

edited

Loading

kristremblay commented May 1, 2021 •

edited

Loading

amaidah commented May 1, 2021 •

edited

Loading