feat(browse): Add support for browsing new releases #425
Conversation
Adds 'New releases' to the browse playlists directory, allowing users to browse Spotify's new album releases via spotify:playlists:new-releases. - Add 'New releases' entry to _PLAYLISTS_DIR_CONTENTS - Extend _browse_playlists() to handle 'new-releases' variant - Add Google-style docstring documenting the function - Add logging for unknown playlist variants - Add comprehensive tests with API documentation references Closes mopidy#241
jodal
left a comment
jodal
left a comment
There was a problem hiding this comment.
Thanks for taking the time to add this feature!
| """Browse playlist-related directories (featured playlists, new releases). | ||
| Args: | ||
| web_client: The Spotify OAuth client for API requests. | ||
| variant: The playlist variant to browse ('featured' or 'new-releases'). | ||
| Returns: | ||
| A list of Mopidy Ref objects (playlists for 'featured', albums for | ||
| 'new-releases'). | ||
| """ |
There was a problem hiding this comment.
I think this comment is better encoded as type hints:
def _browse_playlists(
web_client: SomeTypeIDidntLookupNow,
variant: Literal["featured", "new-releases",
) -> list[Ref]:| items = flatten( | ||
| [ | ||
| page.get("playlists", {}).get("items", []) | ||
| for page in web_client.get_all( | ||
| "browse/featured-playlists", | ||
| params={"limit": 50}, | ||
| ) | ||
| if page | ||
| ] | ||
| ) |
There was a problem hiding this comment.
I realize that you're following the pattern of existing code, but I'm leaving a comment anyway:
I think you can drop the call to flatten() if you just change page.get(...) to *page.get(...). If you do the same with the other calls to flatten() in this file, we can get rid of the util function.
|
|
||
| if variant != "featured": | ||
| return [] | ||
| if variant == "featured": |
There was a problem hiding this comment.
If the function is typed with variant as a Literal, that makes the if variant = ... statements a prime candidate for a match variant: instead. We're now requiring a new enough Python version for match to be used freely 🎉
There was a problem hiding this comment.
Understood. Thank you for the explanation, I'm still learning Python patterns and the API.
|
|
||
|
|
||
| def test_browse_playlists_directory(provider): | ||
| """Test browsing the playlists directory returns all playlist categories.""" |
There was a problem hiding this comment.
In general, skip comments if they don't add anything, e.g. important reasoning for why something is the way it is.
|
|
||
|
|
||
| def test_browse_new_releases(web_client_mock, web_album_mock_base, provider): | ||
| """Test browsing new releases returns album refs. |
There was a problem hiding this comment.
I think most of these comments can be removed without loosing anything. Superflous comments adds content that gets outdated if not maintained, so they are not free.
| assert "Unknown URI type" in caplog.text | ||
|
|
||
|
|
||
| # Defensive programming tests - verifying robust handling of edge cases |
There was a problem hiding this comment.
I think this set of tests takes things a bit far. Having one parametrized test that tests a few of these cases is okay, but we don't need to spend 80 lines of code on testing every possible permutation of broken responses.
There was a problem hiding this comment.
When I say "parametrized test", I'm referring to this Pytest feature: https://docs.pytest.org/en/stable/how-to/parametrize.html#parametrize-basics
There was a problem hiding this comment.
I think this set of tests takes things a bit far. Having one parametrized test that tests a few of these cases is okay, but we don't need to spend 80 lines of code on testing every possible permutation of broken responses.
Thanks for the feedback! I appreciate you taking the time to review this thoroughly.
I hear you on the test coverage. Coming from a Node background, I tend toward comprehensive test cases as documentation and regression protection, but I recognize that may not align with this project's patterns. I'll convert these to a parametrized test as you've suggested - it's a good opportunity for me to learn the preferred testing approach here.
Thanks again for the guidance.
|
If I might be so bold: while I'm doing this would you mind if I take a stab at converting |
2 participants
Adds support for browsing Spotify's new releases via
spotify:playlists:new-releases.Closes #241
Changes
spotify:playlistsbrowse/new-releasesSpotify API endpointTesting
All existing tests pass, plus added tests for:
Screenshots
Iris UI:
API Response:
{ "result": [ {"name": "Taylor Swift - THE TORTURED POETS DEPARTMENT", "type": "album", "uri": "spotify:album:1Mo4aZ8pdj6L1jx8zSwJnt"}, {"name": "Pearl Jam - Dark Matter", "type": "album", "uri": "spotify:album:7MNrrItJpom6uMJWdT0XD8"}, {"name": "Lucky Daye - HERicane", "type": "album", "uri": "spotify:album:4YQ8O3PQb7cZnnLeqNPaa1"}, ... ] }Note to Maintainers
I considered making _browse_playlists more SRP compliant and to mirror the structure of
_browse_album()and_browse_artist()from the get go, but I did not want to complicate this PR. Happy to submit another that takes care of that after this is merged, or include it in this. Let me know.