-
Notifications
You must be signed in to change notification settings - Fork 243
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
Update schedule.go to accept a context.Context; fix fatal bug #307
Conversation
While adding `context.Context` support to the `schedule.go` file, I found a `nil` map write in PreviewSchedule(). This makes me believe this method may be unused, otherwise I would expect a panic report. Updates #267
v, err := query.Values(o) | ||
if err != nil { | ||
return err | ||
} | ||
var data map[string]Schedule | ||
data["schedule"] = s |
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.
These two lines being removed are the bug I am referring to.
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.
Nice catch! I have been able to reproduce the bug you mentioned and verified that this approach no longer panics.
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.
LGTM! And thanks for the bug fix as well! 🎉 🎉
v, err := query.Values(o) | ||
if err != nil { | ||
return err | ||
} | ||
var data map[string]Schedule | ||
data["schedule"] = s |
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.
Nice catch! I have been able to reproduce the bug you mentioned and verified that this approach no longer panics.
While adding
context.Context
support to theschedule.go
file, I found anil
map write in PreviewSchedule(). This makes me believe this method may beunused, otherwise I would expect a panic report.
Updates #267