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

handle spotify web URLs as well #379

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

squeed
Copy link

@squeed squeed commented Feb 23, 2023

This automatically converts spotify web URLs (e.g. https://open.spotify.com/track/XXX) to spotify URIs.

This is particularly useful to me, since I have a NFC-tag jukebox for the kids to use. And it's much easier to write a standard URL to an NFC tag rather than changing it to a spotify URI.

Thanks!

@squeed
Copy link
Author

squeed commented Mar 9, 2023

Thanks for kick-starting CI. That doesn't seem like an issue caused by this PR; should I take a look?

@squeed
Copy link
Author

squeed commented Apr 4, 2023

@fondberg I've rebased on latest master.

@Leicas
Copy link

Leicas commented Sep 9, 2023

I just updated to fix the device_id casting issue to the latest branch and did not realize I needed this for the NFC-tag jukebox I also use for my kids. Any way I can help integrating this for a merge in master ?

Edit: currently merged with a fork here: https://github.com/Leicas/spotcast can confirm it is working well!
@fcusson could it be merged in a future release ?

@squeed
Copy link
Author

squeed commented Sep 9, 2023

@Leicas I worked around this with the following action template:

service: spotcast.start
data:
  entity_id: media_player.wohnzimmer
  uri: |-
    {% set url = trigger.event.data.url %} {% if url.startswith('http') %}
      {% set parts = url.split('/') %}
      spotify:{{ parts[3] }}:{{ parts[4].split('?')[0] }}
    {% else %}
      {{ url }}
    {% endif %}

Copy link
Collaborator

@fcusson fcusson left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply on this one. Small changes requested, this is mostly to ensure your codes fit in the future rewrite I am working on which, one of its part is improving logging and error management.

@@ -171,6 +171,28 @@ def get_random_playlist_from_category(spotify_client:spotipy.Spotify, category:s

return chosen['uri']


def url_to_spotify_uri(url: str) -> str:
Copy link
Collaborator

@fcusson fcusson Apr 14, 2024

Choose a reason for hiding this comment

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

do not return None on error, Raise an error instead to let the parent process decide what to do on error

Copy link
Author

Choose a reason for hiding this comment

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

Sure, makes sense. What's the convention around error types for the project?

@@ -206,6 +207,9 @@ def start_casting(call: ha_core.ServiceCall):
# remove ? from badly formatted URI
uri = uri.split("?")[0]

if uri.startswith("https"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

raise logger error on invalid url. This should be a specific error message different than the URI. The current process could lead people to believe URL are not trully compatible and force them to revert to URI, instead of fixing the url

@squeed
Copy link
Author

squeed commented Apr 24, 2024

Updated, PTAL. Thanks!

This automatically converts spotify web URLs (e.g.
https://open.spotify.com/track/XXX) to spotify URIs.

Signed-off-by: Casey Callendrello <[email protected]>
Copy link
Collaborator

@fcusson fcusson left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants