krystal / go-katapult

Go client library for Katapult
https://pkg.go.dev/github.com/krystal/go-katapult
MIT License
3 stars 1 forks source link

Simplify/improve katapult.Client methods #99

Closed jimeh closed 3 years ago

jimeh commented 3 years ago

I'd like to propose a change to the katapult.Client type with a goal of simplifying it, and making it easier to mock with an interface.

Currently the client has two methods:

This means to mock the Client in other packages, you need to create an interface with both methods, and also implement both in your fakeClient, ideally in such a way that it can verify if the request object is one that came from the client.

Instead, I would like to propose the Client type only has one method:

func (c *Client) Do(r *Request, v interface {}) (*Response, error)

This requires the introduction of a new katapult.Request type, which would contain a few pieces of additional Katapult-specific metadata, as well as what a http.Request struct already contains. And then the Do method becomes responsible for setting various headers, like user-agent, authorization, etc.

This means a fakeClient in other packages only needs to implement the Do method, and simply verify that the correct attributes are set on the given *katapult.Request argument.

I would expect the Request struct to be something like this:

type Request struct {
    http.Request

    // Authenticated determines if the "Authorization" header should be sent.
    // Set to false for un-authenticated API endpoints.
    Authenticated bool

    // Body is any object which can be marshaled to JSON with json.Marshal().
    Body interface{}
}

func NewRequestWithContext(
    ctx context.Context,
    method string,
    url string,
    body interface{},
) *Request {
    return &Request{
        Request: http.NewRequestWithContext(ctx, method, url, nil),
        Body:    body,
    }
}

(I'm not sure embedding http.Request is the best approach, but good enough as a demo here)

strideynet commented 3 years ago

Looks good to me. I can't see any real advantage to them being split.