Spotify: Play owned/starred playlists by name/by frontend #11887

tringler · 2018-01-24T01:04:03Z

Description:

This PR allows you to play owned/starred playlists by name (or uid/uri) or by frontend without the playlist uid.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io# home-assistant/home-assistant.io#4447

Example entry for `configuration.yaml` (if applicable):

# Play a owned/starred playlist by the name
script:
  script_play_music_starred_playlist:
    sequence:
      - service: media_player.select_source
        data_template:
          source: "{{ source }}"
      - service: media_player.play_media
        data:
          media_content_type: playlist
          media_content_id: 'Top 50 Charts' #or 37i9dQZEVXbJiZcmkrIHGU #or spotify:user:spotifycharts
          entity_id: media_player.spotify

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

tschmidty69 · 2018-02-03T18:07:10Z

To fix the requirements problem in the build you need to checkout your branch in a virtualenv, pip -e install it and rerun the gen_requirements script. You can't just edit the txt file directly since that isn't what tox does.

…assistant into spotify-playlists

tringler · 2018-02-15T16:54:33Z

@andrey-git Could you review my PR please?

dgomes

Good work!

I'm no oficial reviewer, but here is some feedback.

dgomes · 2018-02-18T23:00:30Z

homeassistant/components/media_player/spotify.py

-REQUIREMENTS = ['https://github.com/happyleavesaoc/spotipy/'
-                'archive/%s.zip#spotipy==2.4.4' % COMMIT]
+REQUIREMENTS = ['https://github.com/plamere/spotipy/'
+                'archive/master.zip#spotipy==2.4.4']


Just checked and spotipy is available in pypi in version 2.4.4

No need for tor this hack...

It is needed, because the current PyPi 2.4.4 package does not include this PR: spotipy-dev/spotipy#182

The pypi package is from January 2017.
At least I wanted to upgrade from @happyleavesaoc repo to the original master repo.

Just download the code from pypi and GitHub

This package is 💩... same version and different content :(

Yeah, definitely track the spotipy release instead of my fork. w.r.t. pypi version, issue has been open for awhile: spotipy-dev/spotipy#211

Maybe contacting @plamere directly through email ?

I don't have the power to merge :)

But the guidelines are well defined: only pypi packages in requirements

That means I cannot improve a component which is using a GitHub repo since years? Don't touch that component otherwise you are forced to persuade the pypi owner to update the package or maintain your own package for us.

The reasoning is that the master.zip can change and break stuff, while pypi packages will perdure in time.

Great - Then I will revert to @happyleavesaoc commit .zip 👍

I will leave it as it is for now - If someone wants to merge it, he can just revert the 618e751 and a667e4b

dgomes · 2018-02-18T23:13:27Z

homeassistant/components/media_player/spotify.py

+
+    def save(self):
+        """Save playlists."""
+        with open(self.hass.config.path(PERSISTENCE), 'wt') as file:


Over complicated and "file" is a dangerous variable name, I suggest:

with open(self.hass.config.path(PERSISTENCE), 'wt') as p_file: json.dump(self.playlist,p_file)

Can you suggest how you would simplify it?

look at the code in my comment ...

dgomes · 2018-02-18T23:17:08Z

homeassistant/components/media_player/spotify.py

+        path = self.hass.config.path(PERSISTENCE)
+        if not os.path.isfile(path):
+            return {}
+        with open(path) as file:


Over complicated and "file" is a dangerous variable name, I suggest:

with open(path) as p_file: return json.load(p_file)

dgomes · 2018-02-18T23:21:42Z

homeassistant/components/media_player/spotify.py

+        if playlist is not None:
+            self.playlists[playlist['id']] = playlist
+        self.save()
+        self.playlists = self.load()


Why this need to reload a playlist that was just saved ?

It can be removed, but I just wanted to avoid any differences between json and class data. Which approach would you choose? Just load once at init?

isn't save() enough ?

from your code there is no difference...

But then I need to call load() on init, because the idea was to save it locally to avoid all playlists will be discovered at each restart of HASS.

might be a semantic issue... are you updating the persistent file or the playlist variable ? maybe splitting update into two different methods ?

Why not like that?
https://gist.github.com/tringler/a48e8932e7e2692ce1546364df4cded8

need to indent lines 20:21 and 25:26, but that looks good

houndci-bot · 2018-02-19T19:21:33Z

homeassistant/components/media_player/spotify.py

+    def save(self):
+        """Save playlists."""
+        with open(self.hass.config.path(PERSISTENCE), 'wt') as p_file:
+            json.dump(self.playlist, yp_file)


undefined name 'yp_file'

houndci-bot · 2018-02-19T19:21:33Z

homeassistant/components/media_player/spotify.py

+
+    def save(self):
+        """Save playlists."""
+        with open(self.hass.config.path(PERSISTENCE), 'wt') as p_file:


local variable 'p_file' is assigned to but never used

balloob

I think that the approach is wrong. We should not fetch playlists on every update. That's too intense. I also don't think that we should store the playlists on disk.

They should be fetched on demand when the user passes a playlist to play_media

Also please get rid of the view. I think that if we want such a view, we should make it part of the media player component (but in a separate PR)

tringler · 2018-02-23T10:34:40Z

@balloob
The main idea behind the PR is to provide a list of playlists in the media player component frontend, which is updated dynamically if I add/remove/follow/unfollow a playlist. That I can pass a playlistname to play_media is just a side-benefit.

If I fetch the playlists at init of the component I need to restart HASS everytime I update the playlists in Spotify. If I fetch it on demand at play_media action I cannot show it on the frontend as a list.

If I not store it on disk, I need to use the state machine which is also not wanted. Where I should store the data else?

Which approach should I choose?

I agree that the view should be moved to the media_player component.

balloob · 2018-02-26T07:58:14Z

We should not store playlists at all.

The HTTP view that the media player will register can just get the live data upon request and then pass it on to the client. That way it is only queried when the user needs it (because they opened the media player more info dialog).

When the user passes a playlist to play_media we will query the playlists and find the right one to use.

Neither use case requires us to store playlists.

tringler · 2018-02-26T22:16:37Z

@balloob Allright. I think it's the best if I close that PR.

tringler added 6 commits July 29, 2017 22:53

Merge remote-tracking branch 'refs/remotes/home-assistant/dev' into dev

dac3520

Merge remote-tracking branch 'refs/remotes/home-assistant/dev' into dev

d9cc0b4

Changed requirement to original spotipy repo

a667e4b

Updated requirements

618e751

Spotify: Play owned/starred playlist by name/uri

aec8885

Logging improvements (PEP 3101)

d846753

tringler requested a review from andrey-git as a code owner January 24, 2018 01:04

homeassistant added platform: media_player.spotify cla-signed labels Jan 24, 2018

tringler mentioned this pull request Jan 24, 2018

Spotify: Play owned/starred playlists home-assistant/home-assistant.io#4447

Closed

2 tasks

tringler added 2 commits January 24, 2018 09:41

Removed unused key

b31bfb5

Revoked PEP 3101 annotation

9160610

tringler added 6 commits February 6, 2018 20:14

Updated requirements by script

caabeb9

Merge branch 'spotify-playlists' of https://github.com/tringler/home-…

2c591fd

…assistant into spotify-playlists

Logging bugfix

cb156ed

Fixed pylint error: logging-not-lazy

56bf578

Fixed Flake8 error D204

50d061d

Fixed logging syntax error

287ad24

dgomes requested changes Feb 18, 2018

View reviewed changes

tringler added 2 commits February 19, 2018 20:16

Simplified file handling

5061856

Removed useless load operations

c780090

homeassistant added the cla-signed label Feb 19, 2018

houndci-bot reviewed Feb 19, 2018

View reviewed changes

tringler added 2 commits February 19, 2018 20:24

Fixed typo on def save()

f7b6eb0

Fixed another typo on def save()

f10364d

balloob requested changes Feb 23, 2018

View reviewed changes

tringler closed this Feb 26, 2018

ubnt-marc-khouri mentioned this pull request Feb 27, 2018

States with forecasts home-assistant/architecture#15

Closed

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: spotify and removed platform: media_player.spotify labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spotify: Play owned/starred playlists by name/by frontend #11887

Spotify: Play owned/starred playlists by name/by frontend #11887

tringler commented Jan 24, 2018 •

edited

Loading

tschmidty69 commented Feb 3, 2018

tringler commented Feb 15, 2018

dgomes left a comment

dgomes Feb 18, 2018

tringler Feb 19, 2018

dgomes Feb 19, 2018

happyleavesaoc Feb 19, 2018

dgomes Feb 19, 2018

dgomes Feb 19, 2018

tringler Feb 19, 2018 •

edited

Loading

dgomes Feb 19, 2018 •

edited

Loading

tringler Feb 19, 2018

tringler Feb 19, 2018 •

edited

Loading

dgomes Feb 18, 2018

tringler Feb 19, 2018

dgomes Feb 19, 2018

dgomes Feb 18, 2018

dgomes Feb 18, 2018

tringler Feb 19, 2018

dgomes Feb 19, 2018

tringler Feb 19, 2018

dgomes Feb 19, 2018

tringler Feb 19, 2018

dgomes Feb 19, 2018

houndci-bot Feb 19, 2018

houndci-bot Feb 19, 2018

balloob left a comment

tringler commented Feb 23, 2018 •

edited

Loading

balloob commented Feb 26, 2018

tringler commented Feb 26, 2018 •

edited

Loading

Spotify: Play owned/starred playlists by name/by frontend #11887

Spotify: Play owned/starred playlists by name/by frontend #11887

Conversation

tringler commented Jan 24, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

tschmidty69 commented Feb 3, 2018

tringler commented Feb 15, 2018

dgomes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tringler Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

dgomes Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tringler Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

tringler commented Feb 23, 2018 • edited Loading

balloob commented Feb 26, 2018

tringler commented Feb 26, 2018 • edited Loading

tringler commented Jan 24, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

tringler Feb 19, 2018 •

edited

Loading

dgomes Feb 19, 2018 •

edited

Loading

tringler Feb 19, 2018 •

edited

Loading

tringler commented Feb 23, 2018 •

edited

Loading

tringler commented Feb 26, 2018 •

edited

Loading