search

zmb3 / spotify

A Go wrapper for the Spotify Web API
Apache License 2.0
1.34k stars 284 forks source link

Proposed v2 #144

Closed strideynet closed 3 years ago

strideynet commented 3 years ago

Small to-do list

Changes

context.Context

All operations that cross the network will now take a context.Context as their first parameter.

Functional options handling

Move away from having a fixed struct and instead use functional options e.g https://github.com/strideynet/spotify-go/pull/1/commits/e381bd7cb91dd28019d1505951f191307071dd92

This is more easily expanded in future and doesn't require odd behaviour from consuming code (e.g creating a integer for limit and immediately indirecting it as a psuedo-optional parameter)

Linting and Testing

Linting and testing are now implemented using Github actions and run on commits to master and pull requests.

strideynet commented 3 years ago

@zmb3 , I'd be particularly interested in your thoughts on my proposed interface for optional parameters demonstrated here: https://github.com/strideynet/spotify-go/commit/e381bd7cb91dd28019d1505951f191307071dd92

zmb3 commented 3 years ago

Love it. I will take a closer look at this later today. In the meantime, the v2 folder approach sounds like the right thing to do, and I’d be super happy to add you as a maintainer!

strideynet commented 3 years ago

Perfect. I have a few thoughts/questions:

zmb3 commented 3 years ago

Should we outright throw an error when a developer tries to use an option that is not compatible with an endpoint? As far as I can tell, Spotify ignores additional URL parameters so there's no harm other than potential user confusion.

We've (I've) been too strict with things in the past when Spotify didn't actually care, and that caused issues - so I would say if Spotify accepts the request and doesn't issue an error, then we shouldn't error either.

Are there any other outstanding changes that we should aim to incorporate into this v2 release?

I've never been super happy with the authenticator code. Wondering if we rewrite that or simply remove it and provide examples on how to use the standard OAuth packages to get a client/token. Thoughts? There was also #107 which looked like a nice addition but wasn't backwards compatible so we decided not to merge.

strideynet commented 3 years ago

I think I agree with the work in #107 meaning that library consumers would be more free to provide their own HTTP client/authentication, however, there seems to remain some value in keeping our auth component as that seems to work for most users.

It would be interesting to see if adopting functional options everywhere allows us to have a single version of each method, rather than a Foo and FooOpt variant.

My intention (as shown in the example commit) was to rid us of the FooOpt variants, since we can just use variadic options instead, greatly simplifying the library/reducing code.

zmb3 commented 3 years ago

Cool, I'm on board with keeping the authenticator then, so long as we have a way to also provide your own token or authenticated client should you choose to.

This is looking good. Looks like we still need to move things under a /v2 folder. If you want to call this proposal accepted and start making the changes on a v2 branch, I've made you a maintainer. Thanks for all the help!

strideynet commented 3 years ago

Cool, I'm on board with keeping the authenticator then, so long as we have a way to also provide your own token or authenticated client should you choose to.

This is looking good. Looks like we still need to move things under a /v2 folder. If you want to call this proposal accepted and start making the changes on a v2 branch, I've made you a maintainer. Thanks for all the help!

I'm actually not entirely sure that we have to use a separate folder, it may be possible to do it just with tagging as per https://stackoverflow.com/a/53380636 . This seems simpler to me if we have no intentions of hotfixing old releases.

zmb3 commented 3 years ago

Agree we have no intentions of supporting v1 when this work is complete.

I don't want to have to keep a v2 branch for v2 and let the master branch just sit idle. It looks like we can tag on master for v2, but I'm not entirely sure how that works. Hopefully we won't have to keep updating the v2 tag.