From 23e73d962befca21acdb794a9b992d7bd0c6eb27 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 17 Sep 2018 22:13:50 +0200 Subject: [PATCH] core: Define playlists.delete() return type --- docs/changelog.rst | 3 +++ mopidy/backend.py | 6 ++++++ mopidy/core/playlists.py | 21 +++++++++++++++++---- mopidy/m3u/playlists.py | 3 +++ tests/core/test_playlists.py | 14 +++++++++----- tests/m3u/test_playlists.py | 3 ++- 6 files changed, 40 insertions(+), 10 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 7909f108..ba826e45 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -22,6 +22,9 @@ Feature release. - Core: Fix crash on `library.lookup(uris=[])`. (Fixes: :issue:`1619`, PR: :issue:`1620`) +- Core: Define return value of `playlists.delete()` to be a bool, :class:`True` + on success, :class:`False` otherwise. (PR: :issue:`1702`) + - File: Change default ordering to show directories first, then files. (PR: :issue:`1595`) diff --git a/mopidy/backend.py b/mopidy/backend.py index 7412ccc6..8589804a 100644 --- a/mopidy/backend.py +++ b/mopidy/backend.py @@ -362,10 +362,16 @@ class PlaylistsProvider(object): """ Delete playlist identified by the URI. + Returns :class:`True` if deleted, :class:`False` otherwise. + *MUST be implemented by subclass.* :param uri: URI of the playlist to delete :type uri: string + :rtype: :class:`bool` + + .. versionchanged:: 2.2 + Return type defined. """ raise NotImplementedError diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 3c17a898..cc913725 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -180,22 +180,35 @@ class PlaylistsController(object): If the URI doesn't match the URI schemes handled by the current backends, nothing happens. + Returns :class:`True` if deleted, :class:`False` otherwise. + :param uri: URI of the playlist to delete :type uri: string + :rtype: :class:`bool` + + .. versionchanged:: 2.2 + Return type defined. """ validation.check_uri(uri) uri_scheme = urllib.parse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) if not backend: - return None # TODO: error reporting to user + return False + success = False with _backend_error_handling(backend): - backend.playlists.delete(uri).get() - # TODO: error detection and reporting to user + success = backend.playlists.delete(uri).get() + + if success is None: + # Return type was defined in Mopidy 2.2. Assume everything went + # well if the backend doesn't report otherwise. + success = True + + if success: listener.CoreListener.send('playlist_deleted', uri=uri) - # TODO: return value? + return success def filter(self, criteria=None, **kwargs): """ diff --git a/mopidy/m3u/playlists.py b/mopidy/m3u/playlists.py index fe83f663..c1ffc07e 100644 --- a/mopidy/m3u/playlists.py +++ b/mopidy/m3u/playlists.py @@ -93,6 +93,9 @@ class M3UPlaylistsProvider(backend.PlaylistsProvider): os.remove(self._abspath(path)) except EnvironmentError as e: log_environment_error('Error deleting playlist %s' % uri, e) + return False + else: + return True def get_items(self, uri): path = translator.uri_to_path(uri) diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index c908af6a..af1f4fce 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -141,26 +141,30 @@ class PlaylistTest(BasePlaylistsTest): self.assertFalse(self.sp2.create.called) def test_delete_selects_the_dummy1_backend(self): - self.core.playlists.delete('dummy1:a') + success = self.core.playlists.delete('dummy1:a') + self.assertTrue(success) self.sp1.delete.assert_called_once_with('dummy1:a') self.assertFalse(self.sp2.delete.called) def test_delete_selects_the_dummy2_backend(self): - self.core.playlists.delete('dummy2:a') + success = self.core.playlists.delete('dummy2:a') + self.assertTrue(success) self.assertFalse(self.sp1.delete.called) self.sp2.delete.assert_called_once_with('dummy2:a') def test_delete_with_unknown_uri_scheme_does_nothing(self): - self.core.playlists.delete('unknown:a') + success = self.core.playlists.delete('unknown:a') + self.assertFalse(success) self.assertFalse(self.sp1.delete.called) self.assertFalse(self.sp2.delete.called) def test_delete_ignores_backend_without_playlist_support(self): - self.core.playlists.delete('dummy3:a') + success = self.core.playlists.delete('dummy3:a') + self.assertFalse(success) self.assertFalse(self.sp1.delete.called) self.assertFalse(self.sp2.delete.called) @@ -377,7 +381,7 @@ class DeleteBadBackendsTest(MockBackendCorePlaylistsBase): def test_backend_raises_exception(self, logger): self.playlists.delete.return_value.get.side_effect = Exception - self.assertIsNone(self.core.playlists.delete('dummy:/1')) + self.assertFalse(self.core.playlists.delete('dummy:/1')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') diff --git a/tests/m3u/test_playlists.py b/tests/m3u/test_playlists.py index 7cfa8860..ea78e219 100644 --- a/tests/m3u/test_playlists.py +++ b/tests/m3u/test_playlists.py @@ -94,7 +94,8 @@ class M3UPlaylistsProviderTest(unittest.TestCase): self.assertEqual(uri, playlist.uri) self.assertTrue(os.path.exists(path)) - self.core.playlists.delete(playlist.uri) + success = self.core.playlists.delete(playlist.uri) + self.assertTrue(success) self.assertFalse(os.path.exists(path)) def test_playlist_contents_is_written_to_disk(self):