-
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
Exposing context.Context in exported API without requiring major version bump #267
Comments
@stmcallister happy to chat about this proposal over VC / voice if it's easier to have a high bandwidth conversation. If I take on this work I'll probably need to rely on you for reviews + merges, so I want to make sure we're aligned and I address any concerns you have before we go down the path (since it'll require a few PRs to accomplish). |
Okay, I've spoke with Scott out of band about this and we're aligned on this being the right path forward to avoid introducing breaking changes. I'll start opening the PRs to update each file. |
This also updates the new methods to not return the `*http.Response`, as it doesn't seem needed. Updates #267
This also updates the new methods to not return the `*http.Response`, as it doesn't seem needed. Updates #267
This also updates the new methods to not return the `*http.Response`, as it doesn't seem needed. Updates #267
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
@stmcallister all PRs have been opened for this issue, ready for review! |
All PRs were merged; this is done! 🎉 |
I'm raising this issue in anticipation of #266 being merged without many changes to the implementation.Update: This proposal has been accepted and will be moving forward.
I'd like to provide consumers the ability to cancel outstanding requests using
context.Context
by creating methods within the package that take it as their first argument. We would not replace the current methods, but instead create a*WitContext()
variant of each method so that you'd haveGetUser()
andGetUserWithContext()
. We can accomplish this without code duplication, by having the non-context version call the*WithContext()
variant and passing in acontext.Background()
. This should make the long-term maintenance minimal.Progress Tracking
ability.go
- Update ability.go to accept a context.Context #283addon.go
- Update addon.go to accept a context.Context #294business_service.go
- Update business_service.go to accept a context.Context #297change_events.go
- Update change_events.go to accept a context.Context #284escalation_policy.go
- Update escalation_policy.go to accept a context.Context #303event_v2.go
- Update event_v2.go to accept a context.Context #290extension.go
- Update extensions.go to accept a context.Context #301extension_schema.go
- Update extension_schema.go to accept a context.Context #289incident.go
- Update incident.go to accept a context.Context #309log_entry.go
- Update log_entry.go to accept a context.Context #291maintenance_window.go
- Update maintenance_window.go to accept a context.Context #298notifications.go
- Update notification.go to accept a context.Context #288on_call.go
- Update on_call.go to accept a context.Context #287priorities.go
- Update priorities.go to accept a context.Context #285ruleset.go
- Update ruleset.go to accept a context.Context #306schedule.go
- Update schedule.go to accept a context.Context; fix fatal bug #307service.go
- Update service.go to accept a context.Context #299service_dependency.go
- Update service_dependency.go to accept a context.Context #293tag.go
- Update tag.go to accept a context.Context #300team.go
- Update team.go to accept a context.Context #308user.go
- Update user.go to accept a context.Context #286vendor.go
- Update vendor.go to accept a context.Context #292New Methods / Functions
For new additions / methods to the package, we SHOULD NOT have a
*WithContext()
variant and instead make the default implementation require acontext.Context
. For v2 I believe the*WithContext()
variants should be dropped and thecontext.Context
moved into the main method.I think PR #260 is a good example of something we should update to have a
context.Context
as its first argument (no*WithContext()
variant)Why create such a large API to accomplish this?
Speaking intimately about the origins of this package, it was originally released as
1.0.0
versus a0.x
version which would have allowed us to more freely explore the API. Because we've already had a v1.0.0 and we use Go Modules, it would be a sizable amount of work for consumers to upgrade to v2+ (even if there aren't many changes they need to make to code).As such, I think it's prudent for us to provide this functionality without breaking the API so we can better prepare for a 2.0.0 release that fixes any outstanding concerns we have with the API, so the cost that consumers need to pay is worth it.
Example Code
Diff
The text was updated successfully, but these errors were encountered: