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

Remove query parameters from URI #758

Merged
merged 3 commits into from Dec 17, 2021
Merged

Conversation

fcusson
Copy link
Contributor

@fcusson fcusson commented Dec 16, 2021

#757

This PR adds a split to the _get_id function to make sure an URI provided by the user removes any element following an interrogation mark to only return the id element of the URI.

@Peter-Schorn
Copy link
Contributor

The si query parameter is not part of the Spotify URI at all, so it definitely shouldn't be included in the result of the _get_id function.

@fcusson
Copy link
Contributor Author

fcusson commented Dec 17, 2021

I saw that the current function does filter out queries but only from urls. The update I made to _get_id makes sure to also filter it out from URI.

With the si, Spotify is still able to process the URI and point to the correct playlist, I believe we should make sure to provide a fail proof experience to the potential user of the program, just like the user who reported an issue in the spotcast repo.

@Peter-Schorn
Copy link
Contributor

I saw that the current function does filter out queries but only from urls.

That's because si is a query parameter that should only exist in URLs. How did you end up with spotify:playlist:5rAhD02CGReFtSbinTHxkn?si=968337997b194202 in the first place?

@fcusson
Copy link
Contributor Author

fcusson commented Dec 17, 2021

It seems that some of my users take the url, transform it themself in Uri, but forget to remove the queries from the original link.

I never had that problem personally, that's why we just got an issue last week for that and never had to bother with it before.

If I have some user that can do it, there might be other project with similar issue. Filtering out the query doesn't affect anything other than removing the bad element provided by the user.

@stephanebruckert
Copy link
Member

Yeah, this is more a usage issue as the URI is incorrect, but at the same time it wouldn't hurt to clean up the URI inside spotipy too.

Happy to merge this if you manage to fix the checks!

@stephanebruckert stephanebruckert changed the title correction for issue 757 Remove query parameters from URI Dec 17, 2021
@stephanebruckert stephanebruckert merged commit 0464f4f into spotipy-dev:master Dec 17, 2021
IdmFoundInHim pushed a commit to IdmFoundInHim/spotipy that referenced this pull request Dec 23, 2021
* made _get_id take account of ? in URI

* corrected typo

* changes to _get_id explained
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.

Inconsistant result from playlist URI using Spotify.playlist_tracks
3 participants