Make http requests with context #417
No reviewers
Labels
No Label
has/backport
has/pull
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
No Milestone
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: gitea/go-sdk#417
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "6543/go-sdk:with-ctx"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
close #397
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.What's the minimum version of go we accept for the SDK?
RequestWithContext is go 1.13+
with this it is v1.13 or is there a reason to support older?
@ -84,0 +92,4 @@
err error
)
if c.ctx == nil {
req, err = http.NewRequest(method, c.url+path, body)
At the very least if we are moving towards contexts we should use
context.Background()
rather than these nil-checks.Make http requests with context (optional)to Make http requests with contextA few nits.
Only other nit than those would be personal preference. I like options funcs to start with
With
, butSet
somewhat conveys the same thing.@ -49,3 +49,1 @@
url: strings.TrimSuffix(url, "/"),
accessToken: token,
client: &http.Client{},
func NewClient(url string, options ...func(*Client) error) (*Client, 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.
this is for future problems ... do we realy have to remove it?
@ -53,3 +61,4 @@
}
// NewClientWithHTTP creates an API client with a custom http client
// Deprecated use CustomHTTPClient option
I don't see a
CustomHTTPClient
func@jolheiser done