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

feat: more specific times recurrence #7

Merged

Conversation

orimdominic
Copy link
Contributor

Add two methods for specific recurrence

  1. specific recurrence for day: everyDayAtHourAndMinute(hour, minute)
  2. specific recurrence for month: everyMonthAt(day, hour, minute)

@omarkhairy21
Copy link
Owner

Add two methods for specific recurrence

  1. specific recurrence for day: everyDayAtHourAndMinute(hour, minute)
  2. specific recurrence for month: everyMonthAt(day, hour, minute)

Thanks for contributing again, You may follow up the same convention of fromDay().toDayAt(day, hour) but i did not notice it would not be clear when hour and minutes be as arguments at the same function so it may make the developer confused for moments until figuring out which argument is the hour.

I do suggest to change to be like

  1. For day everyDay().atHour(hour).atMinute(minute)
  2. For Month everyMonth().atDay(day).atHour(hour).atMinute(minute)

I would like to hear your thoughts on that

@orimdominic
Copy link
Contributor Author

Thank you for the feedback.

Your suggestion is how I wanted to implement it because I noticed
from the style in the codebase that all functions have an arity of 1. I wrote
it this way because I was considering that users may not want to call all functions
to completion.

For example, a user may want to do only everyDay().atHour(hour) from
everyDay().atHour(hour).atMinute(minute) or only everyMonth().atDay(day) from
everyMonth().atDay(day).atHour(hour).atMinute(minute); and I did not know
if you will want that allowed.

I will make a PR following your suggested convention and I plan to implement it by
doing something similar to

function everyDay() {
  let expression = "min hour * * *";
  return {
    toString() { // or toExpression
      return "0 0 * * *";
    },
    atHour(_hour) {
      expression = expression.replace("hour", _hour);

      return {
        toString() { // or toExpression
          return expression.replace("min", "0");
        },
        atMinute(_minute) {
          return expression.replace("min", _minute);
        },
      };
    },
  };
}
console.log(everyDay().toString()); // 0 0 * * *
console.log(everyDay().atHour(2).toString()); // 0 2 * * * 
console.log(everyDay().atHour(2).atMinute(30)); // 30 2 * * *

We can also make it strict by making sure that all functions are complete executed,
and then remove the toString function returned by the atHour function.
The toString function returned by everyDay has to remain because there already
exists an everyDay function that returns 0 0 * * *.
A suggestion to bypass this is to call the current function that will be written for this
daily().atHour().atMinute() and the other monthly().onDay().atHour()...

What are your thoughts?

@orimdominic
Copy link
Contributor Author

@omarkhairy21 just mentioning you once again in case you missed my latest comment

@omarkhairy21
Copy link
Owner

Thanks for mentioning and reminder i apologize i was very busy last week i pretty much agree with you except use toString we can replace it with get
so it would be like that

console.log(everyDay().atHour(2).get()); // 0 2 * * * 

@orimdominic
Copy link
Contributor Author

Thank you for the feedback @omarkhairy21
I've updated the PR.
Kindly check it out when you can.

@omarkhairy21
Copy link
Owner

omarkhairy21 commented Jun 29, 2023

Thanks, Orim Great work! I did add a small fix for months starting from 0 to 12 (13 months lol!..my mistake) but that standard was 1-12.

I will create a new issue to improve the readability of the test description. if you can work on that.

@omarkhairy21 omarkhairy21 self-requested a review June 29, 2023 14:39
Copy link
Owner

@omarkhairy21 omarkhairy21 left a comment

Choose a reason for hiding this comment

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

Geat @orimdominic thanks.

@omarkhairy21 omarkhairy21 merged commit 0f91280 into omarkhairy21:master Jun 29, 2023
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.

2 participants