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

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

Merged
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 jgrau commented Oct 30, 2021

@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.

Loading

Copy link
Owner

@bensheldon bensheldon left a comment

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 🙌🏻

Loading

@fugit ||= begin
parsed_cron = Fugit.parse(cron)

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

@bensheldon bensheldon Oct 30, 2021

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.

Loading

Copy link
Contributor Author

@jgrau jgrau Oct 30, 2021

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.

Loading

@jgrau jgrau force-pushed the allow-outside-cron-expressions branch from afe75c2 to 21d63fd Oct 30, 2021
@bensheldon bensheldon merged commit f01e946 into bensheldon:main Oct 30, 2021
15 checks passed
Loading
@bensheldon
Copy link
Owner

@bensheldon bensheldon commented Oct 30, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants