eduolalo / moment-business-days

This is a momentJS plugin that allows you to use only business days (Monday to Friday)
MIT License
240 stars 67 forks source link

monthBusinessWeeks() still returning user defined holidays #88

Closed adummy832 closed 3 years ago

adummy832 commented 3 years ago

The title says it all and I think user defined holidays shoudn't be included on "monthBusinessWeeks"

Snippet

const holidays = [
  "01-01-2021",
  "02-12-2021", "02-25-2021",
  "04-01-2021", "04-02-2021", "04-03-2021", "04-04-2021", "04-09-2021",
  "05-01-2021", "05-13-2021",
  "06-12-2021",
  "07-20-2021",
  "08-21-2021", "08-30-2021",
  "11-01-2021", "11-02-2021", "11-30-2021",
  "12-08-2021", "12-24-2021", "12-25-2021", "12-30-2021", "12-31-2021",
]
moment.updateLocale('ph', { holidays, holidayFormat: 'MM-DD-YYYY' })

const date = '01-01-2021' // User defined date
const weeks = moment(date, 'MM-DD-YYYY').monthBusinessWeeks()
console.log(weeks)

Tools

mcdado commented 3 years ago

Hello, I created https://github.com/kalmecak/moment-business-days/pull/91 to address this issue.

Can you tell me if this is behavior that you'd expect?

ZoranderTheRealOne commented 3 years ago

If i right understand that function, it should return array of weeks of array of working days. So for example in my location in january офт2021 Red days are holiday. I change a little getBusinessWeeks function

 while (!done) {
    /*if (day.day() >= 1 && day.day() < 6)*/ if (day.isBusinessDay()){
      daysArr.push(day.clone());
    }

add result be come to

(6) [Array(0), Array(0), Array(5), Array(5), Array(5), Array(0)]
0: []
1: []
2: (5) [k, k, k, k, k]
3: (5) [k, k, k, k, k]
4: (5) [k, k, k, k, k]
5: []
length: 6
__proto__: Array(0)

No It correctly calculate customs holidays.

PS: I think also need check for non zero for last week of month

      if (daysArr.length > 0 && daysArr.length < 5) {
        weeksArr.push(daysArr);
      }
mcdado commented 3 years ago

Yes, that's the change I made, see here: https://github.com/kalmecak/moment-business-days/pull/91/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L207

The question is: do you want also empty weeks or not? What do you think makes more sense?

ZoranderTheRealOne commented 3 years ago

For me empty array is very informative. It gives number of weeks and if array empty it is mean that whole week is holidays. In my country ussualy first week of january is holliday.

ZoranderTheRealOne commented 3 years ago

HI, play a little bit with function and i have a extra empty week at the end of array. I changed condition for adding last week

  var weeksCount = end.diff(startDate,'weeks', true);
  weeksCount = (weeksCount > 0)?Math.ceil(weeksCount):Math.floor(weeksCount);
....
    if (end.diff(day.add(1, 'd')) < 0) {
      /*if (daysArr.length < 5) */
      if (weeksArr.length < weeksCount){
        weeksArr.push(daysArr);
      }
      done = true;
    }