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

Jump to bottom

Allow for cron schedules to be expressed using fugit natural language parsing #441

Merged
merged 4 commits into from Oct 30, 2021
Merged

Allow for cron schedules to be expressed using fugit natural language parsing #441

merged 4 commits into from Oct 30, 2021

Conversation

jgrau
Copy link
Contributor

@jgrau jgrau commented Oct 30, 2021

@bensheldon something like this? Please feel free to nitpick!

Closes #439

@jgrau jgrau changed the title Allow outside cron expressions Allow for cron schedules to be expressed using fugit natural language parsing Oct 30, 2021
@jgrau
Copy link
Contributor Author

jgrau commented Oct 30, 2021 •
edited

@bensheldon I did two iterations on this: the first that assumed the user would supply the Fugit::Cron instance as the configuration and the second using Fugit.parse which works with both the cron-style and natural language.

^ That's the reason for the 2 commits.

bensheldon
bensheldon approved these changes Oct 30, 2021
View reviewed changes
Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

This is fantastic! I like the version that still takes a string, but with a broader syntax. I left one comment about surfacing the ArgumentError earlier in execution.

Let me know your thoughts on that and I'll merge this 🙌🏻

lib/good_job/cron_entry.rb Outdated
@fugit ||= begin
parsed_cron = Fugit.parse(cron)

raise ArgumentError, "Invalid cron format: '#{cron}'" unless parsed_cron.instance_of?(Fugit::Cron)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this ArgumentError would be clearer if placed in #initialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree :) Updated. And fixed linting errors.

@jgrau jgrau force-pushed the allow-outside-cron-expressions branch from afe75c2 to 21d63fd Compare October 30, 2021 21:31
@bensheldon bensheldon merged commit f01e946 into bensheldon:main Oct 30, 2021
@bensheldon bensheldon added the enhancement New feature or request label Oct 30, 2021
@bensheldon
Copy link
Owner

@jgrau Thank you! 🎉 This has been released in GoodJob v2.6.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bensheldon bensheldon bensheldon approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to express cron schedule using fugit natural language parser
2 participants
@jgrau @bensheldon