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

Fix a small OAuth bug #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bojdell
Copy link

@bojdell bojdell commented Nov 7, 2016

Thanks for making rspotify @guilhermesad !

I'm using this library on a side project, and I noticed that there is something weird going on with the OAuth code. Specifically, it is defining the REST verbs once when the module is instantiated, which happens before the @client_token is fetched. However, the verb definitions aren't updated later when @client_token is present, which causes all the requests to fail. I can't get it to work without this patch.

Similarly, wanted to make sure that a proper error message is communicated when a User is instantiated without an id (was following the comments here https://github.com/guilhermesad/rspotify#notes ).

I tried to keep my changes as minimally invasive as possible, though I think there's quite a decent amount refactoring that could be done to make the code less reliant on mutable state (it's pretty confusing to read at first glance).

Let me know if you have any questions / comments please!

@guilhermesad
Copy link
Owner

Hi @bojdell, thanks for the contribution! The reason why the REST methods don't need to be updated is because inside of them the @client_token is always dynamically retrieved. So after you authenticate once, @client_token is set and then whenever you run any REST method it'll be checked in runtime.

Regarding the error message for User, that sounds good, we can keep it. Please let me know if you have any questions/remarks. Thanks!

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.

2 participants