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

Search, view tracks websocket endpoints, service to like media. #489

Merged
merged 45 commits into from
Dec 14, 2024

Conversation

mikevanes
Copy link

@mikevanes mikevanes commented Nov 28, 2024

Adds a generic get method which can call any method on the Spotify API. Used to retrieve views in the Spotify card. This abuses the Spotipy _get method.

Please have a look at the one comment I made which from my point of view is strange behavior.

Doesn't make use of the build in pager as this would require significant refactoring of that method and is imo for now not worth it. I see it as very unlikely that someone would need more than 50playlists displayed.

EDIT

This branch was largely refactored. It adds multiple websocket endpoints:

  • liked_media_handler =>to retrieve all liked songs of a user.
  • search_handler => to search a playlist, track etc.
  • view_handler => to retrieve a view, e.g. recently-played
  • tracks_handler => retrieves a list of tracks from a playlist

Also adds an service to like a song.

@mikevanes mikevanes marked this pull request as ready for review November 28, 2024 15:51
@fcusson
Copy link
Collaborator

fcusson commented Nov 28, 2024

@mikevanes keep up the good work on your PR, but with the changes I'll have to implement to get the account properly working using both the Public and Private APIs I'll have a lot of refactor to do. Keep working from the dev branch, I'll work from a feature branch and I'll integrate your changes cleanly in the new architecture (I might actually do a full rewrite of the Spotify Account class. There are some drawbacks I see from the original implementation. I want to move to DataCoordinators and also I see a lot of code repeat I can probably remove.

@mikevanes mikevanes changed the title Adds a generic get method Adds a search and a view endpoints to the websocket. Nov 29, 2024
locale=locale,
platform=platform,
types=types,
limit=limit,
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the _async_pager for anything relying on offset and limits

Copy link
Author

Choose a reason for hiding this comment

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

I used the pager but see if you agree on the solution. This was the only way I saw it would work.

@@ -952,6 +948,36 @@ async def async_generic_playlists(
return await self.hass.async_add_executor_job(
partial(self._internal_cont._get, url, None, **params)
)

async def async_search(
Copy link
Collaborator

Choose a reason for hiding this comment

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

THere is already an async_search method. This method is generalised for any type of search. But you would have to build a SearchQuery object to utilize it

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@fcusson
Copy link
Collaborator

fcusson commented Nov 29, 2024

Hi @mikevanes, just saw you added (or overwritten) a async_search function. The current impolementation is supposed to be generic and should already suite the use case you seem to be trying to implement.

If you have any issue with the current async_search, please let discuss, because I don't want it to remove functionality from the play_from_search service.

If you need help understanding how it works and how to build a valid SearchQuery to send to the function, I can also help you with that

@fcusson fcusson marked this pull request as draft November 29, 2024 14:45
@fcusson
Copy link
Collaborator

fcusson commented Nov 29, 2024

@mikevanes. Converted to draft to not receive notification on each push. Please move to ready for review when you are

@mikevanes
Copy link
Author

mikevanes commented Nov 29, 2024

@fcusson im sorry for spamming you :) I hadn’t seen the generic search function, I’ll use that tomorrow doesn't seem that difficult, but I am no python developer.

The async pager won’t work I’m afraid for the view method as the _get method which is called for a completely different signature from all the other spotipy methods. Or atleast I couldn't figure out how to get that working.

@fcusson
Copy link
Collaborator

fcusson commented Nov 29, 2024

@mikevanes no stress, wouldn't call it spam.

Can you send me an example reply. I'll see if there is a way to make it work in the pager

@mikevanes mikevanes marked this pull request as ready for review November 29, 2024 15:18
@mikevanes mikevanes marked this pull request as draft November 29, 2024 15:22
@mikevanes
Copy link
Author

@fcusson I fixed the comments you mentioned, please see if you agree on my solution for the pager in the view method. In the code I explained why I did it this way. For now this PR is done for me.

@mikevanes mikevanes marked this pull request as ready for review November 30, 2024 12:18
@fcusson
Copy link
Collaborator

fcusson commented Nov 30, 2024

@mikevanes seems, good, I requested a small change, this is to make it simpler to unit test the code.

Also, I want to ensure I maintain 100% coverage of the code with test. I will need some unit-test on the functions. Either you can look at writing them, or please provide examples of responses for different use cases so that I can create test on them.

@mikevanes
Copy link
Author

@fcusson I have added the documentation for all endpoints. If it is easy to do, it would be nice to have a sensor in HA that provides the current song with id, name and icon uri.

@mikevanes mikevanes marked this pull request as ready for review December 12, 2024 08:25
@mikevanes mikevanes changed the title Adds a search and a view endpoints to the websocket. Search, view tracks websocket endpoints, service to like media. Dec 12, 2024
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.

Added some small change to harmonize documentation. I'll take care of the issues with hassfest. I'll send a new pr to your fork

custom_components/spotcast/spotify/account.py Outdated Show resolved Hide resolved
- spotify:track:2dbJ0mn0vTVz6mc3rk2t77
account: 01JDG07KSBTYWZGJSBJ1EW6XEF
```
### `spotify_uris` (list of str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ensure blank space between sections

use python style type definition list[str]


The result of the transaction.

#### `total` (int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add quote style indentation for sub level definition see raw account doc for example

{
"id": 15,
"type": "spotcast/tracks",
"playlist_id": "37i9dQZF1DXcBWIGoYBM5M" or "spotify:playlist:37i9dQZF1DXcBWIGoYBM5M",
Copy link
Collaborator

Choose a reason for hiding this comment

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

only show one consistent example. I would prefer we show example with uri. The definition of playlist_id is sufficient to provide the rest


The result of the transaction.

#### `total` (int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

add quote indentation


The result of the transaction.

#### `total` (int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

add quote indentation

@fcusson
Copy link
Collaborator

fcusson commented Dec 12, 2024

fixed Hassfest fails. Should be good to go, but I had to bump the minimal version to 2024.12 due to a change in the options flow

@mikevanes
Copy link
Author

mikevanes commented Dec 14, 2024

@fcusson The documentation should be fixed now. You can merge your branch as far as I am concerned

@mikevanes mikevanes requested a review from fcusson December 14, 2024 09:25
@fcusson
Copy link
Collaborator

fcusson commented Dec 14, 2024

@mikevanes can you please approve my pr on your fork. We'll need it to pass validation

@fcusson
Copy link
Collaborator

fcusson commented Dec 14, 2024

will deal with test failing in dev

@fcusson fcusson merged commit b9c19df into fondberg:dev Dec 14, 2024
2 of 4 checks passed
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