search

zmb3 / spotify

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

Add AuthURLWithDialog to force app authentication with Spotify #95

Closed linux-jedi closed 5 years ago

linux-jedi commented 5 years ago

94 Added a new showDialog boolean to the Authenticator struct and a setter method for it. For now, it is initialized to false by default. I didn't want to break the existing API/flow, so I didn't add it to the constructor or existing configuration methods. I would love any feedback/suggestions/changes as I am not super familiar with Go code bases :)

zmb3 commented 5 years ago

I would probably not worry about breaking the API and would instead make showDialog an argument to AuthURL. Alternatively, you could add a new method AuthURLWithDialog.

linux-jedi commented 5 years ago

Can we merge these changes?

zmb3 commented 5 years ago

Sorry @linux-jedi - didn't see your update. Just one small request - can we not export the show dialog constant? (I'm not convinced we need a constant at all since there's only one use, but if we must have it, lets at least keep it internal to the package.)

linux-jedi commented 5 years ago

I went ahead and just removed the constant. I agree – it is only being used once, so there's no real need for it.

zmb3 commented 5 years ago

Nice, thanks. I think we can remove the show dialog Boolean arg as well since there’s no need to have the toggle there. If you don’t want the dialog you just call the original method instead.

linux-jedi commented 5 years ago

Nice catch. Totally was not thinking when I changed it from my original approach.

zmb3 commented 5 years ago

Thanks @linux-jedi! Sorry for the delay.