Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend expected http.StatusAccepted with http.StatusNoContent (closes #99) #100

Closed
wants to merge 2 commits into from

Conversation

Eun
Copy link

@Eun Eun commented Aug 2, 2019

closes #99

@Eun
Copy link
Author

Eun commented Aug 3, 2019

Will fix these tests asap.

@Eun
Copy link
Author

Eun commented Aug 5, 2019

Ready to merge

@zmb3
Copy link
Owner

zmb3 commented Aug 5, 2019

Can we just make execute smart enough to treat all 2XX codes as successful so we don't have to pass http.StatusAccepted to every call?

@Eun
Copy link
Author

Eun commented Aug 6, 2019

Accepting all 200 codes feels a bit risky, instead I would rather just add the specified 200 codes from spotify:

https://developer.spotify.com/documentation/web-api/#response-status-codes

@zmb3
Copy link
Owner

zmb3 commented Aug 6, 2019

Sure, but the reason we're in this situation is that Spotify sometimes sends 200 codes that it doesn't document, and thus this library issues an error when the operation succeeded..

@Eun
Copy link
Author

Eun commented Aug 8, 2019

I haven't seen 200 codes that are undocumented. I can extend execute to treat 200, 201, 202 and 204 as success like they say here.

@zmb3
Copy link
Owner

zmb3 commented Sep 30, 2019

@Eun any follow up on extending execute as mentioned above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using spotifyd
3 participants