Exposing context.Context in exported API without requiring major version bump

[theckman]

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 have GetUser() and GetUserWithContext(). We can accomplish this without code duplication, by having the non-context version call the *WithContext() variant and passing in a context.Background(). This should make the long-term maintenance minimal.

Progress Tracking

• ability.go - Update ability.go to accept a context.Context #283
• addon.go - Update addon.go to accept a context.Context #294
• business_service.go - Update business_service.go to accept a context.Context #297
• change_events.go - Update change_events.go to accept a context.Context #284
• escalation_policy.go - Update escalation_policy.go to accept a context.Context #303
• event_v2.go - Update event_v2.go to accept a context.Context #290
• extension.go - Update extensions.go to accept a context.Context #301
• extension_schema.go - Update extension_schema.go to accept a context.Context #289
• incident.go - Update incident.go to accept a context.Context #309
• log_entry.go - Update log_entry.go to accept a context.Context #291
• maintenance_window.go - Update maintenance_window.go to accept a context.Context #298
• notifications.go - Update notification.go to accept a context.Context #288
• on_call.go - Update on_call.go to accept a context.Context #287
• priorities.go - Update priorities.go to accept a context.Context #285
• ruleset.go - Update ruleset.go to accept a context.Context #306
• schedule.go - Update schedule.go to accept a context.Context; fix fatal bug #307
• service.go - Update service.go to accept a context.Context #299
• service_dependency.go - Update service_dependency.go to accept a context.Context #293
• tag.go - Update tag.go to accept a context.Context #300
• team.go - Update team.go to accept a context.Context #308
• user.go - Update user.go to accept a context.Context #286
• vendor.go - Update vendor.go to accept a context.Context #292

New Methods / Functions

For new additions / methods to the package, we SHOULD NOT have a *WithContext() variant and instead make the default implementation require a context.Context. For v2 I believe the *WithContext() variants should be dropped and the context.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 a 0.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

// GetUser gets details about an existing user. It's recommended to use
// GetUserWithContext instead.
func (c *Client) GetUser(id string, o GetUserOptions) (*User, error) {
	return c.GetUserWithContext(context.Background(), id, o)
}

// GetUserWithContext gets details about an existing user.
func (c *Client) GetUserWithContext(ctx context.Context, id string, o GetUserOptions) (*User, error) {
	v, err := query.Values(o)
	if err != nil {
		return nil, err
	}

	resp, err := c.get(ctx, "/users/"+id+"?"+v.Encode())
	return getUserFromResponse(c, resp, err)
}

Diff

diff --git a/user.go b/user.go
index d232b1c..daa999b 100644
--- a/user.go
+++ b/user.go
@@ -117,13 +117,19 @@ func (c *Client) DeleteUser(id string) error {
 	return err
 }
 
-// GetUser gets details about an existing user.
+// GetUser gets details about an existing user. It's recommended to use
+// GetUserWithContext instead.
 func (c *Client) GetUser(id string, o GetUserOptions) (*User, error) {
+	return c.GetUserWithContext(context.Background(), id, o)
+}
+
+// GetUserWithContext gets details about an existing user.
+func (c *Client) GetUserWithContext(ctx context.Context, id string, o GetUserOptions) (*User, error) {
 	v, err := query.Values(o)
 	if err != nil {
 		return nil, err
 	}
-	resp, err := c.get(context.TODO(), "/users/"+id+"?"+v.Encode())
+	resp, err := c.get(ctx, "/users/"+id+"?"+v.Encode())
 	return getUserFromResponse(c, resp, err)
 }

[theckman]

@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).

[theckman]

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.

[theckman]

@stmcallister all PRs have been opened for this issue, ready for review!

[theckman]

All PRs were merged; this is done! 🎉