Context in requests - beginning of a breaking change discussion

[orian]

It should be possible to add context.Context to requests. It's particularly useful for a request we want to control a timeout, so one can use context.WithTimeout().

Not sure about how to make it as the current implementation assumes that options is just query.Values.
The go' standard way is to pass context as first argument.

[dlebech]

@orian This is a good suggestion. I'm curious what you mean by "The go' standard way is to pass context as first argument.". As far as I know, the standard way is to call Request.WithContext(...) when you want to add context. There are no standard POST, GET etc. functions that take context as a parameter. Perhaps I'm missing something or misunderstanding what you mean?

Here's a suggestion in the meantime: If you want to specify e.g. a timeout globally for all your Shopify requests, then you could look into replacing the http.Client being used. It's exposed as the Client field on the object returned from goshopify.NewClient(). Alternatively, you could also experiment with calling the NewRequest and Do functions directly if you need more control.

[orian]

Thanks. I'm aware about client method.

By "standard" I mean that at least few APIs (e.g. gRPC) and context.Context guideline itself suggest to pass context as a first argument.
func (Api) DoSmth(context.Context, ...) (ReturnValue, error) {}

[dlebech]

I understand your point about the "official" recommendation being to always add the context as first argument to functions. Similarly to #92, I'll let this one brew for a few days and see what people think.

Perhaps you have some suggestions on how to best manage a breaking change like this?

[orian]

I think using: http://labix.org/gopkg.in would make the most sense.
We can then leave master at v1 and create v2 with breaking changes if we don't want to break go get for current API users. We should recommend to import through a gopkg.in in future and probably provide an import instruction for those using golang's dep tool (now officially recommended dependence management).

Or we can break the API and provide instructions for current users to switch to gopkg.in/getconversio/go-shopify.v1 and move master forward.

Moving master forward is good idea for future so dep and other more advanced tools always fetch the most recent version.

[dlebech]

I'll be honest and say that I haven't followed the progress on this front, so I'm not familiar with the go dep tool besides seeing it mentioned in various places. I did use gopkg.in before, and I like the idea that it brings to the table, but I have also seen mentions about it being a bottle-neck and potentially not maintained anymore, which creates a bit of a tricky situation :-)

Reading the documentation for go dep, it seems that it will prefer semantic version branches first and then branches like master second. So as I understand it, we could actually avoid breaking changes for now by just creating a new v2.x branch and v2.0.0 tag (this is inspired by the gRPC branch naming). That would work both for existing go get (it would just continue using master) and go dep users. It would also allow people to use gopkg.in if they wish (see my comment below as well).

What do you think? Is this similar to what you had in mind as well?

Ideally, I would like to follow official guidelines about dependency management, so focusing on supporting go dep going forward is most appealing to me :-)

PS. I changed the title of this issue to better reflect this is now kind of a discussion topic. 🍻

[andrewhoff]

This issue was moved to bold-commerce#5