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

Change option IncludeSeconds to RequireSeconds #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdmallen
Copy link
Contributor

@jdmallen jdmallen commented Feb 7, 2021

Hiya. Feel free to ignore this PR if it's not what you had in mind.

For my uses, having to specify via an options file whether I want to include seconds in the cron expression or not is a little tedious, and you already have nearly all the infrastructure in place to handle either 5 or 6 tokens automatically. So this is me giving it that one final push. Now whenever RequireSeconds is set true, the parser will throw only if Seconds are missing. Otherwise, if it's false, it'll accept 5 or 6 tokens without issue.

Sorry for all the renaming. The meat-and-potatoes of this change is a new test method, CrontabScheduleTests.CanParseEitherFormatWhenSecondsAreNotRequired, and the slightly modified implementation of CrontabSchedule.TryParse<T>.

Let me know what you think and if you'd like me to make any changes. Thanks for making this! Very useful :)

Copy link
Owner

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and idea. Since this would mean a breaking change (due to the rename), I am going to hold it off until the next major release.

@jdmallen
Copy link
Contributor Author

jdmallen commented Feb 8, 2021

Thanks for your contribution and idea. Since this would mean a breaking change (due to the rename), I am going to hold it off until the next major release.

No problem, and that totally makes sense. Maybe I can refactor this such that IncludeSeconds is left alone (with expected functionality when true), but introduce a new option that allows either 5 or 6 tokens regardless of the state of IncludeSeconds. It's clumsy, but it wouldn't break things. Maybe AutoDetectSeconds or something. Thoughts?

@atifaziz
Copy link
Owner

@jdmallen Unfortunately, I don't have a ton of time to work on NCrontab, so I'd rather keep my energy reserved for essentials and critical problems. That's where I am drawing the line for now. The kind of adjustments and enhancements you're proposing is something that could easily be added as a layered API on top the existing API, albeit remaining private to a project and not part of a public NuGet package (though it could go into another package). I think when I am in a position to tackle the next major release, this is something I'll definitely return to consider. Hope you'll be around with as much enthusiasm then; if not, it'll be my loss.

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