-
Notifications
You must be signed in to change notification settings - Fork 240
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 APIError struct to use new NullAPIErrorObject type for safety #272
Conversation
e430f7b
to
69aa6f4
Compare
@echlebek I could use your eyes on this one too, since it was your issue that spawned this API. |
f85a8d8
to
168142c
Compare
I discovered that my initial implementation of the APIError type's NotFound() method had the potential for triggering a nil pointer dereference panic, which wouldn't be a great experience for someone dealing with PagerDuty code. I provided a fix in #271, but in doing so I remembered this pattern from the `database/sql` package and decided that, while not as nice to use, it was a much safer API to reduce the chances of the consumer accidentally triggering a panic in their program. Since we've yet to release v1.4.0 (i.e., the new APIError type still hasn't been released) we can still change this without breaking API compatibility guarantees.
168142c
to
5e16250
Compare
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. It does improve the safety of the library to remove the chance of a nil pointer deref.
In terms of ergonomics, I wonder if it wouldn't be a little nicer to make methods on APIError instead? For example:
type APIError struct {
statusCode int
apiError *APIErrorObject
}
func (a APIError) StatusCode() int {
return a.statusCode
}
func (a APIError) ErrorCode() int {
if a.apiError != nil {
return a.apiError.Code
}
return 0
}
func (a APIError) Message() string { ... }
func (a APIError) Errors() []string { ... }
Thank you for your thoughtful reply, I honestly considered it doing that way first and eventually talked myself out of it. The biggest concern I had is that we then still need to have a I also know people work with the |
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.
Good call. Do you want me to wait to cut a new release?
@stmcallister alright; merged it in. Thank you for that approval. I think I'm ready for a v1.4.0 now. |
@stmcallister I can look to prep it if you'd like. It seems like I need to get a PR up for the CHANGELOG.md file, but I would need your approval to merge it. LMK if that sounds good. |
I discovered that my initial implementation of the APIError type's NotFound()
method had the potential for triggering a nil pointer dereference panic, which
wouldn't be a great experience for someone dealing with PagerDuty code.
I provided a fix in #271, but in doing so I remembered this pattern from the
database/sql
package and decided that, while not as nice to use, it was a muchsafer API to reduce the chances of the consumer accidentally triggering a panic
in their program.
Since we've yet to release v1.4.0 (i.e., the new APIError type still hasn't been
released) we can still change this without breaking API compatibility
guarantees.