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

Offset from cron date #120

Open
bilogic opened this issue Jul 5, 2021 · 11 comments
Open

Offset from cron date #120

bilogic opened this issue Jul 5, 2021 · 11 comments
Labels
looking for votes Features requests I'm leaving open to gauge interest

Comments

@bilogic
Copy link

bilogic commented Jul 5, 2021

Hi,

  1. I need to collect on invoices every quarter, i.e. 1st of Jan/Apr/Jul/Oct.
  2. I would like to send out a reminder, say 7 days prior.
  3. I was thinking of adding shift() and dependency on https://github.com/briannesbitt/Carbon
$cron = new Cron\CronExpression('@quarterly');
echo $cron->shift('-7 days')->getNextRunDate(null, 2)->format('Y-m-d H:i:s');
  1. Would this be an acceptable PR?
@bilogic
Copy link
Author

bilogic commented Jul 6, 2021

@dragonmantank would love to hear your opinions :)

@bilogic
Copy link
Author

bilogic commented Jul 11, 2021

@DerekCresswell what do you think? Thanks.

@DerekCresswell
Copy link

Sounds like an interesting addition to me. However, I don't think such a thing would be accepted. The maintainer of the repository considers it feature complete. So such a change is unlikely to be added. I would just say extend the class yourself and add this functionality to it.

@bilogic
Copy link
Author

bilogic commented Jul 12, 2021

@DerekCresswell

Thanks, I mistook you as the maintainer as only your name appeared when I looked through the recent commits.

@bilogic
Copy link
Author

bilogic commented Sep 18, 2021

@dragonmantank possible to say a yes or no? (preferrably yes 🤣)

@dragonmantank
Copy link
Owner

Thanks for the suggestion.

As @DerekCresswell mentioned this library tries to stick to the original C implementation as much as possible, so I'm not sure I would add this. I will gladly leave open for others to see if they would like it though.

@dragonmantank dragonmantank added the looking for votes Features requests I'm leaving open to gauge interest label Jan 5, 2022
@bilogic
Copy link
Author

bilogic commented Jan 5, 2022

Thanks @dragonmantank

How do we vote?

@dragonmantank
Copy link
Owner

I just leave it open for anyone else to chime in. I added the label to make it easier for people to find when looking through issues, because we have a few feature requests that are like this (might make life easier, but diverge from the original implementation).

@nyamsprod
Copy link

@bilogic correct me if I am wrong but can't you not rewrite the code this way ?

$cron = new Cron\CronExpression('@quarterly');
echo $cron->getNextRunDate(null, 2)->sub(DateInterval::createFromDateString('7 days'))->format('Y-m-d H:i:s');

Seems to me that nothing needs to be added to the library to achieve what you are looking for 🤔 Or did I miss something ?

@bilogic
Copy link
Author

bilogic commented Jan 19, 2022

@nyamsprod

I can't remember my exact thought process, but I'm using this library via Laravel's schedule. I vaguely recall being able to expose an instance of this library, and thus wanted to be able to shift(...) it so that when Laravel calls it's getNextRunDate(), it gets the shifted date.

@bilogic
Copy link
Author

bilogic commented Sep 24, 2022

@nyamsprod I revisited this, this repo's isDue() needs to return true for the shifted date. As far as I can tell, your code doesn't do that. Basically the shifting logic must be contained within getNextRunDate() otherwise lots of code needs to be modified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
looking for votes Features requests I'm leaving open to gauge interest
Projects
None yet
Development

No branches or pull requests

4 participants