Skip to content

Conversation

@Sachin24704
Copy link

fix- #5719- Bug Report: Schedule function with minutes does not work #5719

changed the regex expression to match the cron expression

What does this PR do?

It fixes the bug that we get when trying to put in double digits in the schedule cron expression for the function.

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
I checked if this regex expression worked seperately by inputting different values

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you read the Contributing Guidelines on issues?

yes
(Write your answer here.)

…th minutes does not work #5719

changed the regex expression to match the cron expression
@vercel
Copy link

vercel bot commented Jul 4, 2023

@Sachin24704 is attempting to deploy a commit to the appwrite Team on Vercel.

A member of the Team first needs to authorize it.

@Sachin24704
Copy link
Author

appwrite/appwrite#5719

@stnguyen90 stnguyen90 self-requested a review July 18, 2023 17:43
Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great PR! 🤯 Unfortunately, it looks like the unit tests failed. Would you please fix the unit tests and also add test cases for the additional values you added.

let error: string;
const pattern = String.raw`^[^]+\s+[^]+\s+[^]+\s+[^]+\s+[^]+`;
const pattern = String.raw`/(@(annually|yearly|monthly|weekly|daily|hourly|reboot))|(@every (\d+(ns|us|µs|ms|s|m|h))+)|((((\d+,)+\d+|(\d+(\/|-)\d+)|\d+|\*) ?){5,7})/`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Regexes are hard to understand and maintain. Would you be able to try and break this apart and add comments to try and help explain it?

@stnguyen90
Copy link
Contributor

@Sachin24704 are you able to address the comment I made or should I close this?

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