-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add flag for persistant interval #132
Conversation
@@ -147,7 +152,7 @@ func main() { | |||
|
|||
// Create cache | |||
cache := job.NewMemoryJobCache(db) | |||
cache.Start(DefaultPersistEvery) | |||
cache.Start(time.Duration(c.Int("persist-every")) * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the built-in duration string parsing - https://golang.org/pkg/time/#ParseDuration
This allows for 4h
instead of 14400
Would also mean making it a StringFlag
with a default of 5m
.
Lastly lets remove the DefaultPersistEvery = 5 * time.Second
at the top of this file.
duration, err := time.ParseDuration(c.String("persist-every")) | ||
// if duration string could not be parsed, set duration to default | ||
if err != nil { | ||
cache.Start(time.Duration(5 * time.Minute)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This silently fails to parse what the user specified and does something else instead. Not sure what @ajvb thinks should happen here, but hiding the error doesn't seem like the right way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have two options:
- Let the program crash if the duration string could not be parsed and show an error.
- Show a message that the duration string could not be parsed and that the persistant interval was set to the default value.
Hey @rotespferd, thank you so much for the PR. I decided to go with #133 as a starting place. |
Add flag as described in #131