Make http requests with context #417

If we are going to add contexts, we should at least use context.Background() if the user doesn't set their own. That way there's less nil-checking and fewer footguns.

This might also be a good opportunity to add the functional options to the NewClient func.

If we are going to add contexts, we should at least use `context.Background()` if the user doesn't set their own. That way there's less nil-checking and fewer footguns. This might also be a good opportunity to add the functional options to the `NewClient` func.

zeripath commented

2020-09-14 19:28:44 +00:00

What's the minimum version of go we accept for the SDK?

RequestWithContext is go 1.13+

What's the minimum version of go we accept for the SDK? RequestWithContext is go 1.13+

6543 added

status/wip

and removed

status/needs-reviews

labels 2020-09-14 22:42:57 +00:00

6543 commented

2020-09-15 03:49:56 +00:00

What's the minimum version of go we accept for the SDK?

with this it is v1.13 or is there a reason to support older?

> What's the minimum version of go we accept for the SDK? with this it is v1.13 or is there a reason to support older?

jolheiser requested changes 2020-09-15 14:16:39 +00:00

Dismissed

gitea/client.go Outdated

						
				@ -84,0 +92,4 @@

						err error

					)

					if c.ctx == nil {

						req, err = http.NewRequest(method, c.url+path, body)

jolheiser commented

2020-09-15 14:16:33 +00:00

At the very least if we are moving towards contexts we should use context.Background() rather than these nil-checks.

At the very least if we are moving towards contexts we should use `context.Background()` rather than these nil-checks.

6543 changed title from ~~Make http requests with context (optional)~~ to Make http requests with context

2020-09-15 14:51:35 +00:00

6543 added 4 commits 2020-09-15 14:51:43 +00:00

4e3ad433c0

Use Functional Options fo NewClient

4ded091354

Add Migration Docu

3b8507def1

always use Context

continuous-integration/drone/pr Build is failing

Details

ee0e4fa5de

add SetSudo

6543 added 1 commit 2020-09-15 14:56:14 +00:00

continuous-integration/drone/pr Build is failing

Details

9062e028a0

md format

jolheiser requested changes 2020-09-15 15:02:05 +00:00

Dismissed

jolheiser left a comment

A few nits.

Only other nit than those would be personal preference. I like options funcs to start with With, but Set somewhat conveys the same thing.

A few nits. Only other nit than those would be personal preference. I like options funcs to start with `With`, but `Set` somewhat conveys the same thing.

gitea/client.go Outdated

						
				@ -49,3 +49,1 @@

						url:         strings.TrimSuffix(url, "/"),

						accessToken: token,

						client:      &http.Client{},

				func NewClient(url string, options ...func(*Client) error) (*Client, error) {

jolheiser commented

2020-09-15 14:56:57 +00:00

It doesn't look like any of the functional optional actually return an error?

I think this might be an attempt at fixing a future problem (which is fine), but imo setting an option shouldn't be able to inherently throw an error.

It doesn't look like any of the functional optional actually return an error? I think this might be an attempt at fixing a future problem (which is fine), but imo setting an option shouldn't be able to inherently throw an error.

6543 commented

2020-09-15 15:12:04 +00:00

this is for future problems ... do we realy have to remove it?

gitea/client.go Outdated

						
				@ -53,3 +61,4 @@

				}

				// NewClientWithHTTP creates an API client with a custom http client

				// Deprecated use CustomHTTPClient option