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

Implements markets endpoint #161

Merged
merged 4 commits into from
May 5, 2023
Merged

Conversation

hayribakici
Copy link
Collaborator

@hayribakici hayribakici commented Apr 18, 2023

This PR implements the v1/markets endpoint. In order to prevent errors as such, the country codes are stored in an enum

@hayribakici hayribakici requested a review from rinukkusu April 18, 2023 17:01
@rinukkusu
Copy link
Owner

Not a big fan of the CountryCode class. Way too much logic and code for what Spotify exposes. We simply get back a (list of) string. We don't need to convert between different ISO representations. I'm not sure it's even necessary to abstract this result more.

What would be the gain of such a class?

@hayribakici
Copy link
Collaborator Author

hayribakici commented Apr 23, 2023

Not a big fan of the CountryCode class. Way too much logic and code for what Spotify exposes. We simply get back a (list of) string. We don't need to convert between different ISO representations. I'm not sure it's even necessary to abstract this result more.

What would be the gain of such a class?

Pretty much abstraction and preventing errors when handling with country codes, just as we use the enums in our library. What do you think about an enum with country codes?

@hayribakici
Copy link
Collaborator Author

@rinukkusu I changed the result-type to an enum. I wanted to keep a fixed structure to prevent any type of error. This could also make the usage of the library easier by putting a Market parameter into certain methods:

BundledPages get(
    String searchQuery, {
    Iterable<SearchType> types = SearchType.all,
    Market? market,
  }) {...

Of course I can change it, when you still think it is unnecessary. 😆

@rinukkusu
Copy link
Owner

Hi, sorry for being MIA - I was on vacation 😆
I think the enum is a very good middleground! And of course, then we should also use it in other places.

Copy link
Owner

@rinukkusu rinukkusu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rinukkusu rinukkusu merged commit 5cfdabd into rinukkusu:master May 5, 2023
@hayribakici hayribakici deleted the markets branch May 5, 2023 12:30
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