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

Authorization scope #156

Merged
merged 4 commits into from
Apr 8, 2023
Merged

Conversation

hayribakici
Copy link
Collaborator

@hayribakici hayribakici commented Apr 8, 2023

This PR provides constants for the authorization scopes. With this, choosing the correct scope will be less error prone as it might happen that the user of this library makes spelling mistakes.

It also adds some refactoring with classes that contain a key property. I looked into it a little closer and the public key property should never be used outside of this library. Therefore it was removed.

I'm not 100% sure of this approach, dividing the scopes into different classes as they also could be fit into the AuthorizationScope class. What do you think?

@hayribakici hayribakici requested a review from rinukkusu April 8, 2023 02:39
@rinukkusu
Copy link
Owner

rinukkusu commented Apr 8, 2023

Thank you for taking the time - I too think the idea of adding constants for all scopes is a great addition and makes it less error prone for consumers of the library!

Nevertheless I think the current implementation is a little confusing as it's not entirely clear which scope is hiding behind which class. IMHO they can all live in one class with the full scope identifier as member name e.g. 'user-library-read' becomes AuthorizationScope.userLibraryRead and so on. Like that we don't introduce another layer of complexity and are on the same level as the Spotify WebAPI.

Edit: I've thought about it again and I think your approach makes it easier for people not looking for the scope identifiers, because the separation makes sense, but harder for people looking for scope identifiers 😆. Still I really like how it sensibly ties in with how the library itself is being used, so let's keep your implementation and maybe add a Wiki entry. What do you think @hayribakici ?

@rinukkusu rinukkusu merged commit 11ef149 into rinukkusu:master Apr 8, 2023
@hayribakici hayribakici deleted the authorization_scope branch April 8, 2023 20:22
@hayribakici
Copy link
Collaborator Author

Thank you. Yeah, a wiki entry sounds reasonable. Do you think the wiki page should just list all the scopes and for which methods they should be used or do you have something else in mind?

I would go even further and maybe put the Authorization Code Flow entry in the README.md also in the wiki, as this information is quite detailed regarding the usage of this libarary. What do you think?

@hayribakici hayribakici restored the authorization_scope branch April 8, 2023 20:27
@rinukkusu
Copy link
Owner

Do you think the wiki page should just list all the scopes and for which methods they should be used or do you have something else in mind?

I wouldn't even go that far ... only if you feel like doing the extra work, that's something the official documentation of the Spotify WebAPI already covers anyway, no? I'd just list our class segments and which scopes you can find where. Like that a consumer of the library could easily Ctrl + F to search for the scope identifier.

I would go even further and maybe put the Authorization Code Flow entry in the README.md also in the wiki, as this information is quite detailed regarding the usage of this libarary. What do you think?

Sure, sounds like a good idea! 👍

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