zmb3 / spotify

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

Added support for adding and removing albums from a user's library #62

Closed ddevcap closed 3 years ago

ddevcap commented 6 years ago

Updated the album_test. This was the only test that i could find where it would fail if the requested object wasn't a nil pointer. All other test are looking at the err. So i removed that assertion to make it behave like the other tests. Err should always be checked before continuing, right? What do you think?

ddevcap commented 6 years ago

Or revert those changes and update the tests ;)

a-schuurman commented 6 years ago

Nice PR! I need this too :) Do you guys have an update on this?

zmb3 commented 6 years ago

Sorry for losing track of this.

I wasn't super clear and for that I'm sorry. In my original comment, I suggested changes like the following:

var result []bool
err := c.get(spotifyURL, &result)
return result, err

or

return c.execute(req, nil)

The key here, especially in the first example is that in the case of an error, result should still be nil. I think that behavior is important to preserve, as most APIs behave this way, and ones that don't typically document it explicitly.

I'm fine with either backing out to your first commit and merging that, or having you go back through and re-evaluate the error checking that was unrelated to the original feature. What do you think?

ddevcap commented 6 years ago

I see! It would indeed be a breaking change if we return an empty struct. Altho it is a general good practice to check for errors before using the returned object. Also, if you where not to check for the error you would then get a runtime error because you're trying to access a property that does not exists on the object. So returning an empty struct would also prevent that right?

But i completely see your point. I'll revert the changes and commit as you mentioned: "go back through and re-evaluate the error checking"

strideynet commented 3 years ago

Closing as exceptionally behind master.