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 not allowing from token as market/country #179

Merged
merged 7 commits into from
Feb 7, 2021
Merged

Fix not allowing from token as market/country #179

merged 7 commits into from
Feb 7, 2021

Conversation

Qluxzz
Copy link
Contributor

@Qluxzz Qluxzz commented Jan 25, 2021

Description

Fixes "from_token" not being a valid option for market/country.

Create Market enum which can be a country or from_token in order to satisfy both conditions.

Update usages of Country to instead use Market enum.

Update mentions and parameters of country/market to be consistent with spotify API docs.

Fixes #28

Motivation and Context

To allow market to be automatically selected from the users access-token instead of having to be supplied manually.

Dependencies

Type of change

Please delete options that are not relevant.

  • This change requires a documentation update
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Please also list any relevant details for your test configuration:
I ran all tests but the most interesting ones would be the following since they used the old Country enum

  • Market::Country: test_artist_top_tracks
  • Market::Country: test_categories
  • Market::Country: test_category_playlists
  • Market::Country: test_recommendations
  • Market::Country: test_search_artist

I've also added a test that uses Market::FromToken functionality, but since the market supplied is not static and changes with the oauth token credentials, it maybe wouldn't be such a good test?

  • Market::FromToken: test_search_artist_with_from_token

src/model/enums/misc.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@marioortizmanero marioortizmanero left a comment

Choose a reason for hiding this comment

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

I think the test you added is OK, since we can't check the results anyway. As long as it makes sure it doesn't break it's enough for now.

src/client.rs Outdated Show resolved Hide resolved
src/model/enums/misc.rs Outdated Show resolved Hide resolved
src/model/enums/misc.rs Outdated Show resolved Hide resolved
@ramsayleung
Copy link
Owner

Is this PR is ready to merge, it seems there is no more update after the last commit?

@Qluxzz
Copy link
Contributor Author

Qluxzz commented Feb 7, 2021

Yes, I implemented the to_string trait instead of into trait as @marioortizmanero asked me about, and then requested a review.

@marioortizmanero
Copy link
Collaborator

Yes sorry! I was busy but I'm finally done with finals. Merging this, thanks for the contribution :)

@marioortizmanero marioortizmanero merged commit eadb957 into ramsayleung:master Feb 7, 2021
@Qluxzz
Copy link
Contributor Author

Qluxzz commented Feb 7, 2021

@marioortizmanero no worries, hope you aced your finals!

@Qluxzz Qluxzz deleted the fix-not-allowing-from-token branch February 7, 2021 10:47
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.

"from_token" is currently not a valid parameter
3 participants