diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b349af5..7d711cb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Unified `__repr__` format to ClassName(key=value) +- For all services, the `get_playlist` method is now more consistent and always returns None if no playlist is found, independent of the used identifier. Added `get_playlist_or_raise` to get predictable behaviour and return Type. ## [0.4.0] - 2026-03-07 diff --git a/plistsync/core/collection.py b/plistsync/core/collection.py index 74372262..3e7e4beb 100644 --- a/plistsync/core/collection.py +++ b/plistsync/core/collection.py @@ -401,9 +401,16 @@ def get_playlist(self, *args, **kwargs) -> C | None: """Get a playlist by identifier. Implement with kwargs like ``name=``, ``id=``, ``url=``, or ``uri=``. - Return ``None`` for name searches that fail. + Return ``None`` for searches that fail. """ ... + def get_playlist_or_raise(self, *args, **kwargs) -> C: + """Like get_playlist() but raises if no result is found.""" + playlist = self.get_playlist(*args, **kwargs) + if playlist is None: + raise ValueError(f"Could not find playlist for {kwargs}") + return playlist + def __repr__(self) -> str: return f"{type(self).__name__}()" diff --git a/plistsync/services/plex/api.py b/plistsync/services/plex/api.py index b00df60a..aa9f4081 100644 --- a/plistsync/services/plex/api.py +++ b/plistsync/services/plex/api.py @@ -652,7 +652,7 @@ def section_name_to_id(self, section_name_or_id: str | int) -> int: return section_id raise ValueError(f"Library '{section_name_or_id}' not found.") - def playlist_name_to_id(self, playlist_name_or_id: str | int) -> int: + def playlist_name_to_id(self, playlist_name_or_id: str | int) -> int | None: """Resolve a playlist ID from a name or return the ID if already numeric.""" try: playlist_id = int(playlist_name_or_id) @@ -672,5 +672,6 @@ def playlist_name_to_id(self, playlist_name_or_id: str | int) -> int: ) return playlist_id - raise ValueError(f"Playlist '{playlist_name_or_id}' not found.") + log.debug(f"Playlist '{playlist_name_or_id}' not found.") + return None return playlist_id diff --git a/plistsync/services/plex/library.py b/plistsync/services/plex/library.py index e3f6a8e5..e4a1a41f 100644 --- a/plistsync/services/plex/library.py +++ b/plistsync/services/plex/library.py @@ -3,7 +3,7 @@ from pathlib import Path from typing import Any, overload -from typing_extensions import override +from requests import HTTPError from plistsync.core import GlobalTrackIDs, LibraryCollection, PathRewrite from plistsync.core.collection import GlobalLookup, LocalLookup, TrackStream @@ -84,11 +84,9 @@ def playlists(self) -> Iterable[PlexPlaylistCollection]: @overload def get_playlist(self, *, name: str) -> PlexPlaylistCollection | None: ... - @overload - def get_playlist(self, *, id: int) -> PlexPlaylistCollection: ... + def get_playlist(self, *, id: int) -> PlexPlaylistCollection | None: ... - @override def get_playlist( self, name: str | None = None, @@ -96,11 +94,10 @@ def get_playlist( ) -> PlexPlaylistCollection | None: """Get a specific playlist. - One of the kwargs must be given. Either search - by name or get by id (rating_key). + Exactly one of the kwargs must be given. Either search + by name or by id (rating_key). - Will raise on id not found but return None if - search by name not found. + Will return None if not found. Tracks are fetched eagerly. """ @@ -111,17 +108,17 @@ def get_playlist( id = self.api.converts.playlist_name_to_id(name) if id is None: - # For searches we want to return None if not found - log.debug(f"Could not find playlist with name '{name}'") return None - plist = PlexPlaylistCollection.from_response_data( - library=self, - playlist_data=self.api.playlist.get(id), - tracks_data=self.api.playlist.get_items(id), - ) - - return plist + try: + return PlexPlaylistCollection.from_response_data( + library=self, + playlist_data=self.api.playlist.get(id), + tracks_data=self.api.playlist.get_items(id), + ) + except HTTPError as e: + log.debug(f"Failed to get playlist for {id=}, likely invalid id: {e}") + return None @cached_property def locations(self) -> list[Path]: diff --git a/plistsync/services/spotify/api.py b/plistsync/services/spotify/api.py index 6683860f..7fc958e6 100644 --- a/plistsync/services/spotify/api.py +++ b/plistsync/services/spotify/api.py @@ -676,7 +676,7 @@ def _get_playlists_full(self) -> list[SpotifyApiPlaylistResponseFull]: return playlists_details -def extract_spotify_playlist_id(url_or_uri: str) -> str: +def extract_spotify_playlist_id(url_or_uri: str) -> str | None: """Extract the Spotify ID from a playlist URL or URI.""" # Pattern matches: # spotify:playlist: @@ -690,4 +690,5 @@ def extract_spotify_playlist_id(url_or_uri: str) -> str: if match: return match.group(1) else: - raise ValueError(f"Invalid Spotify playlist URL or URI: {url_or_uri}") + log.debug(f"Invalid Spotify playlist URL or URI: {url_or_uri}") + return None diff --git a/plistsync/services/spotify/library.py b/plistsync/services/spotify/library.py index 4365ce8b..2f50d40c 100644 --- a/plistsync/services/spotify/library.py +++ b/plistsync/services/spotify/library.py @@ -2,7 +2,6 @@ from typing import overload from requests import HTTPError -from typing_extensions import override from plistsync.core import GlobalTrackIDs from plistsync.core.collection import ( @@ -46,17 +45,13 @@ def playlists(self) -> Iterable[SpotifyPlaylistCollection]: @overload def get_playlist(self, *, name: str) -> SpotifyPlaylistCollection | None: ... - @overload - def get_playlist(self, *, id: str) -> SpotifyPlaylistCollection: ... - + def get_playlist(self, *, id: str) -> SpotifyPlaylistCollection | None: ... @overload - def get_playlist(self, *, url: str) -> SpotifyPlaylistCollection: ... - + def get_playlist(self, *, url: str) -> SpotifyPlaylistCollection | None: ... @overload - def get_playlist(self, *, uri: str) -> SpotifyPlaylistCollection: ... + def get_playlist(self, *, uri: str) -> SpotifyPlaylistCollection | None: ... - @override def get_playlist( self, name: str | None = None, @@ -66,11 +61,9 @@ def get_playlist( ) -> SpotifyPlaylistCollection | None: """Get a specific playlist. - One of the kwargs must be given. Either search - by name or get by id/url/uri. + Exactly one of the kwargs must be given: name/id/url/uri. - Will raise on id/url/uri not found but return None if - search by name not found. + Returns None if not found. """ if sum(arg is not None for arg in [name, id, url, uri]) != 1: @@ -89,19 +82,19 @@ def get_playlist( id = plist["id"] break - if id is None: - # For searches we want to return None if not found - log.debug(f"Could not find playlist with name '{name}'") - return None - - # This should never realistically happen -> assert instead of error - assert id is not None, "ID must be set after resolving name/url/uri" + if id is None: + log.debug(f"Could not find playlist for {name=} {uri=} {url=}") + return None # Direct ID lookup (fastest path) - return SpotifyPlaylistCollection.from_response_data( - self, - self.api.playlist.get(id), - ) + try: + return SpotifyPlaylistCollection.from_response_data( + self, + self.api.playlist.get(id), + ) + except HTTPError as e: + log.debug(f"Failed to get playlist for {id=}, likely invalid id: {e}") + return None # --------------------------- GlobalLookup protocol -------------------------- # diff --git a/plistsync/services/tidal/api.py b/plistsync/services/tidal/api.py index 738394b8..5c770203 100644 --- a/plistsync/services/tidal/api.py +++ b/plistsync/services/tidal/api.py @@ -802,7 +802,7 @@ def include_to_lookup(included: list[T_Included]) -> LookupDict[T_Included]: return {(item["type"], item["id"]): item for item in included} -def extract_tidal_playlist_id(url: str) -> str: +def extract_tidal_playlist_id(url: str) -> str | None: """Extract the Tidal playlist ID from a URL.""" # Example URL formats: # https://tidal.com/browse/playlist/{playlist_id} @@ -815,4 +815,5 @@ def extract_tidal_playlist_id(url: str) -> str: if match: return match.group(1) else: - raise ValueError(f"Invalid Tidal playlist URL: {url}") + log.debug(f"Invalid Tidal playlist URL: {url}") + return None diff --git a/plistsync/services/tidal/library.py b/plistsync/services/tidal/library.py index 5a0ae74c..fc1ca849 100644 --- a/plistsync/services/tidal/library.py +++ b/plistsync/services/tidal/library.py @@ -1,6 +1,8 @@ from collections.abc import Iterable from typing import overload +from requests import HTTPError + from plistsync.core import GlobalTrackIDs from plistsync.core.collection import ( GlobalLookup, @@ -34,12 +36,10 @@ def playlists(self) -> Iterable[TidalPlaylistCollection]: @overload def get_playlist(self, *, name: str) -> TidalPlaylistCollection | None: ... - @overload - def get_playlist(self, *, id: str) -> TidalPlaylistCollection: ... - + def get_playlist(self, *, id: str) -> TidalPlaylistCollection | None: ... @overload - def get_playlist(self, *, url: str) -> TidalPlaylistCollection: ... + def get_playlist(self, *, url: str) -> TidalPlaylistCollection | None: ... def get_playlist( self, @@ -49,11 +49,9 @@ def get_playlist( ) -> TidalPlaylistCollection | None: """Get a specific playlist. - One of the kwargs must be given. Either search - by name or get by id/url. + Exactly one of the kwargs must be given: name/id/url. - Will raise on id/url not found but return None if - search by name not found. + Returns None if not found. """ if sum(arg is not None for arg in [name, id, url]) != 1: raise ValueError("Exactly one of name, id, or url must be provided") @@ -69,18 +67,24 @@ def get_playlist( ) found = [p for p in playlists if p["attributes"]["name"] == name] if len(found) == 0: - log.info(f"Could not find playlist with name {name}") - return None + id = None + else: + id = found[0]["id"] - id = found[0]["id"] if len(found) > 1: log.info(f"Found more than one playlist with name {name}, using {id}") - # This should never realistically happen -> assert instead of error - assert id is not None, "ID must be set after resolving name/url" - return TidalPlaylistCollection.from_response_data( - self, *self.api.playlist.get(id) - ) + if id is None: + log.debug(f"Could not find playlist for {name=} {url=}") + return None + + try: + return TidalPlaylistCollection.from_response_data( + self, *self.api.playlist.get(id) + ) + except HTTPError as e: + log.debug(f"Failed to get playlist for {id=}, likely invalid id: {e}") + return None def has_playlist(self, name: str) -> bool: """Check if a playlist with the given name exists in the user's library.""" diff --git a/plistsync/services/traktor/library.py b/plistsync/services/traktor/library.py index 0915c0ce..57b41564 100644 --- a/plistsync/services/traktor/library.py +++ b/plistsync/services/traktor/library.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from collections.abc import Iterable from datetime import datetime from pathlib import Path @@ -14,7 +16,7 @@ from .path import NMLPath from .playlist import NMLPlaylistCollection from .track import NMLTrack -from .utility import xpath_string_escape +from .utility import sanitize_plist_name, xpath_string_escape if TYPE_CHECKING: from lxml.etree import _Element, _ElementTree @@ -31,7 +33,7 @@ class NMLLibraryCollection(LibraryCollection, TrackStream, LocalLookup): """ path: Path - tree: "_ElementTree" + tree: _ElementTree def __init__(self, path: Path | str): if isinstance(path, str): @@ -81,23 +83,21 @@ def playlists(self) -> Iterable[NMLPlaylistCollection]: yield pl @overload - def get_playlist(self, *, name: str) -> NMLPlaylistCollection: ... + def get_playlist(self, *, name: str) -> NMLPlaylistCollection | None: ... @overload - def get_playlist(self, *, uuid: str) -> NMLPlaylistCollection: ... + def get_playlist(self, *, uuid: str) -> NMLPlaylistCollection | None: ... def get_playlist( self, name: str | None = None, uuid: str | None = None, - # path: str | Path | None = None, - ) -> NMLPlaylistCollection: + ) -> NMLPlaylistCollection | None: """Get a specific playlist. - One of the kwargs must be given. Either search - by name or get by uuid. + Exactly one of the kwargs must be given. Either search by name or by uuid. - Will raise if not found! + If Ids are not found this raises, but if names are not found it retuns None. """ if sum(arg is not None for arg in [name, uuid]) != 1: raise ValueError("Exactly one of name or uuid must be provided") @@ -106,17 +106,26 @@ def get_playlist( if uuid is not None: root_node = self._get_playlist_root_node_by_uuid(uuid) - else: - root_node = self._get_playlist_root_node_by_name(name) # type: ignore[arg-type] + elif name is not None: + s_name = sanitize_plist_name(name) + if s_name != name: + log.warning( + f"Playlist name changed from `{name}` to `{s_name}`" + " to avoid issues with Traktor.", + ) + root_node = self._get_playlist_root_node_by_name(s_name) + + if root_node is None: + return None return NMLPlaylistCollection(self, root_node) - def _playlist_nodes(self) -> "Iterable[_Element]": + def _playlist_nodes(self) -> Iterable[_Element]: """Get all playlists in the NML file.""" nodes = self.tree.xpath(".//NODE[@TYPE='PLAYLIST']") return nodes - def _get_playlist_root_node_by_uuid(self, uuid: str) -> "_Element": + def _get_playlist_root_node_by_uuid(self, uuid: str) -> _Element | None: """Get a playlist by uuid.""" node = self.tree.xpath( @@ -125,16 +134,16 @@ def _get_playlist_root_node_by_uuid(self, uuid: str) -> "_Element": if len(node) > 0: return node[0] else: - raise ValueError(f"Playlist '{uuid}' not found!") + return None - def _get_playlist_root_node_by_name(self, name: str) -> "_Element": + def _get_playlist_root_node_by_name(self, name: str) -> _Element | None: node = self.tree.xpath( f".//NODE[@TYPE='PLAYLIST'][@NAME={xpath_string_escape(name)}]" ) if len(node) > 0: return node[0] else: - raise ValueError(f"Playlist '{name}' not found!") + return None # --------------------------- LocalLookup protocol --------------------------- # diff --git a/plistsync/services/traktor/playlist.py b/plistsync/services/traktor/playlist.py index cc576001..90cd54da 100644 --- a/plistsync/services/traktor/playlist.py +++ b/plistsync/services/traktor/playlist.py @@ -64,7 +64,7 @@ def __init__( if s_name != name: log.warning( f"Playlist name changed from `{name}` to `{s_name}`" - " issues with Traktor.", + " to avoid issues with Traktor.", ) root_node = self._create_playlist_node(s_name) else: @@ -211,11 +211,10 @@ def _remote_create(self): @property def remote_associated(self) -> bool: - try: - self.library._get_playlist_root_node_by_uuid(self.uuid) - return True - except ValueError: + root_node = self.library._get_playlist_root_node_by_uuid(self.uuid) + if root_node is None: return False + return True def remote_upsert(self): """Insert or replace a playlist node in this NML library. diff --git a/tests/abc.py b/tests/abc.py index dc090388..f5bd842b 100644 --- a/tests/abc.py +++ b/tests/abc.py @@ -185,12 +185,12 @@ def known_playlists(self) -> Iterable[tuple[str, Any]]: @property @abstractmethod - def unknown_playlists(self) -> Iterable[tuple[str, Any, bool]]: - """Unknow playlist for lookup by [key, value, expect_raise]. + def unknown_playlists(self) -> Iterable[tuple[str, Any]]: + """Unknow playlist for lookup by [key, value]. - E.g. ["uri", "spotify:not_found",True] - will call get_playlist(uri="spotify:asdasdasd") - and expects it to raise! + E.g. ["uri", "spotify:not_found"] + will call get_playlist(uri="spotify:asdasdasd") -> check None + and get_playlist_or_raise(uri="spotify:asdasdasd") -> check raises """ pass @@ -214,15 +214,14 @@ def test_get_playlist_known(self): def test_get_playlist_unknown(self): """Test retrieval of unknown playlists by name or identifier.""" for library_collection in self.create_collection(): - for key, identifier, expect_raise in self.unknown_playlists: - if expect_raise: - ctxm: Any = pytest.raises(ValueError) - else: - ctxm = nullcontext() - - with ctxm: - playlist = library_collection.get_playlist(**{key: identifier}) - assert playlist is None, "Unknown playlist should not be found" + for key, identifier in self.unknown_playlists: + playlist = library_collection.get_playlist(**{key: identifier}) + assert playlist is None, "Unknown playlist should not be found" + + with pytest.raises(ValueError): + playlist = library_collection.get_playlist_or_raise( + **{key: identifier} + ) class PlaylistCollectionTestBase(ABC): diff --git a/tests/services/plex/test_collection.py b/tests/services/plex/test_collection.py index 0571efc2..519bd699 100644 --- a/tests/services/plex/test_collection.py +++ b/tests/services/plex/test_collection.py @@ -75,10 +75,10 @@ def known_playlists(self) -> list[tuple[str, str]]: ] @property - def unknown_playlists(self) -> list[tuple[str, str, bool]]: + def unknown_playlists(self) -> list[tuple[str, str]]: return [ - ("name", "Unknown Playlist", False), - ("name", "Another Unknown Playlist", False), + ("name", "Unknown Playlist"), + ("name", "Another Unknown Playlist"), ] def test_preload(self): diff --git a/tests/services/test_spotify.py b/tests/services/test_spotify.py index a304005d..ff2602cd 100644 --- a/tests/services/test_spotify.py +++ b/tests/services/test_spotify.py @@ -66,5 +66,5 @@ def test_valid_inputs(self, input_str, expected_id): ) def test_invalid_inputs(self, invalid_input): """Test that invalid inputs raise ValueError.""" - with pytest.raises(ValueError, match="Invalid Spotify playlist URL or URI"): - extract_spotify_playlist_id(invalid_input) + id = extract_spotify_playlist_id(invalid_input) + assert id is None diff --git a/tests/services/traktor/test_collection.py b/tests/services/traktor/test_collection.py index 6e50cc58..b71ef9c1 100644 --- a/tests/services/traktor/test_collection.py +++ b/tests/services/traktor/test_collection.py @@ -41,8 +41,8 @@ def known_playlists(self): @property def unknown_playlists(self): return [ - ("name", "unknown playlist", True), - ("uuid", "asdasdas", True), + ("name", "unknown playlist"), + ("uuid", "asdasdas"), ] def test_get_playlist_invalid_args(self, collection): @@ -78,13 +78,13 @@ def test_find_by_path(self): def test_write_persists(self, collection: NMLLibraryCollection) -> None: """Calling write should persist the collection""" new_name = "Updated name" - p = collection.get_playlist(uuid="6868ecd66b354d37a33b965dae7a82e7") + p = collection.get_playlist_or_raise(uuid="6868ecd66b354d37a33b965dae7a82e7") p.name = new_name collection.write() # After reload should be persisteted! reloaded = NMLLibraryCollection(collection.path) - p2 = reloaded.get_playlist(uuid="6868ecd66b354d37a33b965dae7a82e7") + p2 = reloaded.get_playlist_or_raise(uuid="6868ecd66b354d37a33b965dae7a82e7") assert p2.name == new_name def test_find_by_local_ids(self, collection: NMLLibraryCollection): @@ -124,7 +124,7 @@ def test_upsert_new_playlist(self, collection: NMLLibraryCollection) -> None: assert len(list(collection._playlist_nodes())) == count_before + 1 # and it's retrievable via public API - fetched = collection.get_playlist(uuid=pl_collection.uuid) + fetched = collection.get_playlist_or_raise(uuid=pl_collection.uuid) assert fetched.name == "New PL" assert fetched.uuid == pl_collection.uuid @@ -172,6 +172,7 @@ def test_upsert_playlist_replaces_existing_by_uuid_and_removes_old_node( # Grab the actual node currently in the tree old_node = collection._get_playlist_root_node_by_uuid(existing_uuid) + assert old_node is not None old_parent = old_node.getparent() assert old_parent is not None old_index = old_parent.index(old_node) @@ -200,7 +201,7 @@ def test_upsert_playlist_replaces_existing_by_uuid_and_removes_old_node( assert old_parent.index(new_node) == old_index # and public API returns the replaced playlist data - fetched = collection.get_playlist(uuid=existing_uuid) + fetched = collection.get_playlist_or_raise(uuid=existing_uuid) assert fetched.name == "Replaced Name" assert fetched.uuid == existing_uuid @@ -241,7 +242,7 @@ def test_remote_delete( assert not pl_collection.remote_associated - # Second delte should trigger value error + # Second delete should trigger value error with pytest.raises(ValueError): pl_collection.remote_delete() @@ -374,7 +375,7 @@ def test_remote_create(self, collection: NMLLibraryCollection): ) with pytest.raises(ValueError): - collection.get_playlist(name="foo") + collection.get_playlist_or_raise(name="foo") p1.remote_create()