feat:custom-cron interface for own custom cron implimentation#834
feat:custom-cron interface for own custom cron implimentation#834JohnRoesler merged 7 commits intogo-co-op:v2from
Conversation
|
@JohnRoesler what's your thoughts on this implementation, my test cases are getting failed due to combine the two different error into one let me know what kinda changes require in this implementation and what should we do for the error ? if you allow me to do with one error then i will change testcases and test and refine it |
job.go
Outdated
| } | ||
|
|
||
| func (c cronJobDefinition) setup(j *internalJob, location *time.Location, now time.Time) error { | ||
| func (c *cronJobDefinition) IsValid() bool { |
There was a problem hiding this comment.
rather than having the JobDefinition implement the Cron interface, I was envisioning that the cronJobDefinition itself would include the Cron interface as a struct field (it will also need to be added to the internalJob struct). Then, in the setup method of the cronJobDefinition, could grab the Cron interface from the internalJob and utilize that.
The default Cron implementation would be the current one with robfig. Then, add a new JobOption that takes in a Cron interface and sets it.
There was a problem hiding this comment.
This solution keeps us to a single CronJob definition and instead allows altering the Cron implementation via a Job option which can be set for a single job, or globally using the WithGlobalJobOptions method on the scheduler.
There was a problem hiding this comment.
There are few points that i want to discuss :
-
Do i have to make a different Jobdefination function since it has extra field which might cause confusion on the Custom implementation via Job option !
-
Where we are going to initialized the cron ?
func WithCronImplementation(c Cron) JobOption { return func(j *internalJob, _ time.Time) error { j.cron = c return nil } }
func (c cronJobDefinition) setup(j *internalJob, location *time.Location, _ time.Time) error { c.cron = j.cron
i hope this is exactly what you are trying to say
i am confused like initialize job and passing implementation is different thing Job initializing happening in job definition and implementation passing through job options how we can use them together. sorry if i am asking dumb question this pattern is new to me and i like this pattern :)
There was a problem hiding this comment.
@Dojeto No worries on asking questions!
- No, I don't think you need a different JobDefinition. if the implementation doesn't care about the
withSecondsbool it can simply ignore it! - Yeah, a little confusing maybe, I'll try and explain:
The Job itself is "initialized" in the NewJob method on the Scheduler, which in turn just calls the internal method addOrUpdateJob. If you look in that method, you'll see the following order of operations:
- Job options are applied, first global, then job specific so that they win -> here
- Then the jobDefinitions
setupmethod is called https://github.com/go-co-op/gocron/blob/v2/scheduler.go#L742
So, in this case, the Cron interface will be initialized either by the caller and then passed in via the WithCronImplementation JobOption, which in turn sets the Cron onto the internalJob struct. OR, the Cron will be initialized inside the setup method as the default implementation.
Then, in the setup method, IsValid should be called.
|
@JohnRoesler how it is ? test cases getting failed due to change in error, i have check seems like it is working, let me know what's your thoughts on this ? let me know required changes. |
What does this do?
This pr for the new feature of custom cron job, added the cron interface for custom cron implementation
Which issue(s) does this PR fix/relate to?
Resolves #804
List any changes that modify/break current functionality
Errors will be change since from two different error
ErrCronJobParseandErrCronJobInvalidi have change it toErrCronJobInvalidonly one which leads to failure in test cases.Have you included tests for your changes?
No
Did you document any new/modified functionality?
No
example_test.goREADME.mdNotes
This is very straight forward approach i have think this needs more refinements, suggestions appreciated