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
Internal refactor of cron #250
Conversation
This was performed to prepare for graphile#219. No new functionality was implemented as part of this commit.
23604f0
to
4fcaeca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid changes; just a bit of painting the bike shed and we're good to go 👍
src/cronMatcher.ts
Outdated
|
||
/** | ||
* Returns true if the cronItem should fire for the given timestamp digest, | ||
* false otherwise. | ||
*/ | ||
export function cronItemMatches( | ||
cronItem: ParsedCronItem, | ||
cronItem: CrontabRanges, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: this is not a breaking change.
hey @benjie can we get this merged? I'll sort out the CI but still needs an approval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! I've been super busy on the PostGraphile V5 repo and am really behind on GitHub notifications right now. This is looking good 👍
|
||
export const parseCronRangeString = ( | ||
pattern: string, | ||
source: string = "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only add in ${source}
if there is a source; so either remove the = ""
here or handle the source
missing in the error message and make sure parseCrontabRanges
is also happy with no source.
source: string = "", | |
source: string, |
} | ||
const { minutes, hours, dates, months, dows } = parseCrontabRanges( | ||
matches, | ||
const { minutes, hours, dates, months, dows } = parseCronString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be parseCronRangeString
?
Description
This was performed to prepare for proper implementation of #218. No new functionality was
implemented as part of this commit. I will rebase #219 on top of this so that the diff for the new feature is smaller.
Performance impact
should be none
Security impact
unknown
Checklist
yarn lint:fix
passes.yarn test
passes.`