From fddb299ed96ed811173510678014f3cf3ae49c46 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 23 Jan 2014 21:31:18 +0100 Subject: [PATCH] mpd: Update type converters naming and add UINT Also adds tests for RANGE helper. --- mopidy/mpd/protocol/__init__.py | 23 +++++++--- mopidy/mpd/protocol/current_playlist.py | 33 ++++++--------- mopidy/mpd/protocol/playback.py | 34 ++++++--------- tests/mpd/test_commands.py | 56 ++++++++++++++++++------- 4 files changed, 83 insertions(+), 63 deletions(-) diff --git a/mopidy/mpd/protocol/__init__.py b/mopidy/mpd/protocol/__init__.py index 9e85d058..2b0d2cbe 100644 --- a/mopidy/mpd/protocol/__init__.py +++ b/mopidy/mpd/protocol/__init__.py @@ -96,29 +96,40 @@ def load_protocol_modules(): stored_playlists) -def integer(value): +def INT(value): if value is None: raise ValueError('None is not a valid integer') + # TODO: check for whitespace via value != value.strip()? return int(value) -def boolean(value): +def UINT(value): + if value is None: + raise ValueError('None is not a valid integer') + if not value.isdigit(): + raise ValueError('Only positive numbers are allowed') + return int(value) + + +def BOOL(value): if value in ('1', '0'): return bool(int(value)) raise ValueError('%r is not 0 or 1' % value) -def position_or_range(value): +def RANGE(value): # TODO: test and check that values are positive if ':' in value: start, stop = value.split(':', 1) - start = int(start) + start = UINT(start) if stop.strip(): - stop = int(stop) + stop = UINT(stop) + if start >= stop: + raise ValueError('End must be larger than start') else: stop = None else: - start = int(value) + start = UINT(value) stop = start + 1 return slice(start, stop) diff --git a/mopidy/mpd/protocol/current_playlist.py b/mopidy/mpd/protocol/current_playlist.py index 41a4782b..662cac8b 100644 --- a/mopidy/mpd/protocol/current_playlist.py +++ b/mopidy/mpd/protocol/current_playlist.py @@ -49,7 +49,7 @@ def add(context, uri): context.core.tracklist.add(tracks=tracks) -@protocol.commands.add('addid', songpos=protocol.integer) +@protocol.commands.add('addid', songpos=protocol.UINT) def addid(context, uri, songpos=None): """ *musicpd.org, current playlist section:* @@ -68,7 +68,6 @@ def addid(context, uri, songpos=None): - ``addid ""`` should return an error. """ - # TODO: check that songpos is positive if not uri: raise exceptions.MpdNoExistError('No such song') if songpos is not None and songpos > context.core.tracklist.length.get(): @@ -79,7 +78,7 @@ def addid(context, uri, songpos=None): return ('Id', tl_tracks[0].tlid) -@protocol.commands.add('delete', position=protocol.position_or_range) +@protocol.commands.add('delete', position=protocol.RANGE) def delete(context, position): """ *musicpd.org, current playlist section:* @@ -99,7 +98,7 @@ def delete(context, position): context.core.tracklist.remove(tlid=[tlid]) -@protocol.commands.add('deleteid', tlid=protocol.integer) +@protocol.commands.add('deleteid', tlid=protocol.UINT) def deleteid(context, tlid): """ *musicpd.org, current playlist section:* @@ -108,7 +107,6 @@ def deleteid(context, tlid): Deletes the song ``SONGID`` from the playlist """ - # TODO: check that tlid is positive tl_tracks = context.core.tracklist.remove(tlid=[tlid]).get() if not tl_tracks: raise exceptions.MpdNoExistError('No such song') @@ -126,8 +124,7 @@ def clear(context): context.core.tracklist.clear() -@protocol.commands.add( - 'move', position=protocol.position_or_range, to=protocol.integer) +@protocol.commands.add('move', position=protocol.RANGE, to=protocol.UINT) def move_range(context, position, to): """ *musicpd.org, current playlist section:* @@ -137,7 +134,6 @@ def move_range(context, position, to): Moves the song at ``FROM`` or range of songs at ``START:END`` to ``TO`` in the playlist. """ - # TODO: check that to is positive start = position.start end = position.stop if end is None: @@ -145,7 +141,7 @@ def move_range(context, position, to): context.core.tracklist.move(start, end, to) -@protocol.commands.add('moveid', tlid=protocol.integer, to=protocol.integer) +@protocol.commands.add('moveid', tlid=protocol.UINT, to=protocol.UINT) def moveid(context, tlid, to): """ *musicpd.org, current playlist section:* @@ -156,7 +152,6 @@ def moveid(context, tlid, to): the playlist. If ``TO`` is negative, it is relative to the current song in the playlist (if there is one). """ - # TODO: check that tlid and to are positive tl_tracks = context.core.tracklist.filter(tlid=[tlid]).get() if not tl_tracks: raise exceptions.MpdNoExistError('No such song') @@ -177,6 +172,7 @@ def playlist(context): Do not use this, instead use ``playlistinfo``. """ + # TODO: warn about this being deprecated return playlistinfo(context) @@ -202,7 +198,7 @@ def playlistfind(context, tag, needle): raise exceptions.MpdNotImplemented # TODO -@protocol.commands.add('playlistid', tlid=protocol.integer) +@protocol.commands.add('playlistid', tlid=protocol.UINT) def playlistid(context, tlid=None): """ *musicpd.org, current playlist section:* @@ -212,7 +208,6 @@ def playlistid(context, tlid=None): Displays a list of songs in the playlist. ``SONGID`` is optional and specifies a single song to display info for. """ - # TODO: check that tlid is positive if not none if tlid is not None: tl_tracks = context.core.tracklist.filter(tlid=[tlid]).get() if not tl_tracks: @@ -281,7 +276,7 @@ def playlistsearch(context, tag, needle): raise exceptions.MpdNotImplemented # TODO -@protocol.commands.add('plchanges', version=protocol.integer) +@protocol.commands.add('plchanges', version=protocol.INT) def plchanges(context, version): """ *musicpd.org, current playlist section:* @@ -303,7 +298,7 @@ def plchanges(context, version): context.core.tracklist.tl_tracks.get()) -@protocol.commands.add('plchangesposid', version=protocol.integer) +@protocol.commands.add('plchangesposid', version=protocol.INT) def plchangesposid(context, version): """ *musicpd.org, current playlist section:* @@ -327,7 +322,7 @@ def plchangesposid(context, version): return result -@protocol.commands.add('shuffle', position=protocol.position_or_range) +@protocol.commands.add('shuffle', position=protocol.RANGE) def shuffle(context, position=None): """ *musicpd.org, current playlist section:* @@ -344,8 +339,7 @@ def shuffle(context, position=None): context.core.tracklist.shuffle(start, end) -@protocol.commands.add( - 'swap', songpos1=protocol.integer, songpos2=protocol.integer) +@protocol.commands.add('swap', songpos1=protocol.UINT, songpos2=protocol.UINT) def swap(context, songpos1, songpos2): """ *musicpd.org, current playlist section:* @@ -354,7 +348,6 @@ def swap(context, songpos1, songpos2): Swaps the positions of ``SONG1`` and ``SONG2``. """ - # TODO: check that songpos is positive tracks = context.core.tracklist.tracks.get() song1 = tracks[songpos1] song2 = tracks[songpos2] @@ -366,8 +359,7 @@ def swap(context, songpos1, songpos2): context.core.tracklist.add(tracks) -@protocol.commands.add( - 'swapid', tlid1=protocol.integer, tlid2=protocol.integer) +@protocol.commands.add('swapid', tlid1=protocol.UINT, tlid2=protocol.UINT) def swapid(context, tlid1, tlid2): """ *musicpd.org, current playlist section:* @@ -376,7 +368,6 @@ def swapid(context, tlid1, tlid2): Swaps the positions of ``SONG1`` and ``SONG2`` (both song ids). """ - # TODO: check that tlid is positive tl_tracks1 = context.core.tracklist.filter(tlid=[tlid1]).get() tl_tracks2 = context.core.tracklist.filter(tlid=[tlid2]).get() if not tl_tracks1 or not tl_tracks2: diff --git a/mopidy/mpd/protocol/playback.py b/mopidy/mpd/protocol/playback.py index d5100ee1..d18aff50 100644 --- a/mopidy/mpd/protocol/playback.py +++ b/mopidy/mpd/protocol/playback.py @@ -4,7 +4,7 @@ from mopidy.core import PlaybackState from mopidy.mpd import exceptions, protocol -@protocol.commands.add('consume', state=protocol.boolean) +@protocol.commands.add('consume', state=protocol.BOOL) def consume(context, state): """ *musicpd.org, playback section:* @@ -18,7 +18,7 @@ def consume(context, state): context.core.tracklist.consume = state -@protocol.commands.add('crossfade', seconds=protocol.integer) +@protocol.commands.add('crossfade', seconds=protocol.UINT) def crossfade(context, seconds): """ *musicpd.org, playback section:* @@ -27,7 +27,6 @@ def crossfade(context, seconds): Sets crossfading between songs. """ - # TODO: check that seconds is positive raise exceptions.MpdNotImplemented # TODO @@ -89,7 +88,7 @@ def next_(context): return context.core.playback.next().get() -@protocol.commands.add('pause', state=protocol.boolean) +@protocol.commands.add('pause', state=protocol.BOOL) def pause(context, state=None): """ *musicpd.org, playback section:* @@ -114,7 +113,7 @@ def pause(context, state=None): context.core.playback.resume() -@protocol.commands.add('play', tlid=protocol.integer) +@protocol.commands.add('play', tlid=protocol.INT) def play(context, tlid=None): """ *musicpd.org, playback section:* @@ -166,7 +165,7 @@ def _play_minus_one(context): return # Fail silently -@protocol.commands.add('playid', tlid=protocol.integer) +@protocol.commands.add('playid', tlid=protocol.INT) def playid(context, tlid): """ *musicpd.org, playback section:* @@ -239,7 +238,7 @@ def previous(context): return context.core.playback.previous().get() -@protocol.commands.add('random', state=protocol.boolean) +@protocol.commands.add('random', state=protocol.BOOL) def random(context, state): """ *musicpd.org, playback section:* @@ -251,7 +250,7 @@ def random(context, state): context.core.tracklist.random = state -@protocol.commands.add('repeat', state=protocol.boolean) +@protocol.commands.add('repeat', state=protocol.BOOL) def repeat(context, state): """ *musicpd.org, playback section:* @@ -293,8 +292,7 @@ def replay_gain_status(context): return 'off' # TODO -@protocol.commands.add( - 'seek', tlid=protocol.integer, seconds=protocol.integer) +@protocol.commands.add('seek', tlid=protocol.UINT, seconds=protocol.UINT) def seek(context, tlid, seconds): """ *musicpd.org, playback section:* @@ -308,15 +306,13 @@ def seek(context, tlid, seconds): - issues ``seek 1 120`` without quotes around the arguments. """ - # TODO: check tlid is positive tl_track = context.core.playback.current_tl_track.get() if context.core.tracklist.index(tl_track).get() != tlid: play(context, tlid) context.core.playback.seek(seconds * 1000).get() -@protocol.commands.add( - 'seekid', tlid=protocol.integer, seconds=protocol.integer) +@protocol.commands.add('seekid', tlid=protocol.UINT, seconds=protocol.UINT) def seekid(context, tlid, seconds): """ *musicpd.org, playback section:* @@ -325,7 +321,6 @@ def seekid(context, tlid, seconds): Seeks to the position ``TIME`` (in seconds) of song ``SONGID``. """ - # TODO: check that songid and time is positive tl_track = context.core.playback.current_tl_track.get() if not tl_track or tl_track.tlid != tlid: playid(context, tlid) @@ -351,7 +346,7 @@ def seekcur(context, time): context.core.playback.seek(position).get() -@protocol.commands.add('setvol', volume=protocol.integer) +@protocol.commands.add('setvol', volume=protocol.INT) def setvol(context, volume): """ *musicpd.org, playback section:* @@ -364,14 +359,11 @@ def setvol(context, volume): - issues ``setvol 50`` without quotes around the argument. """ - if volume < 0: - volume = 0 - if volume > 100: - volume = 100 - context.core.playback.volume = volume + # NOTE: we use INT as clients can pass in +N etc. + context.core.playback.volume = min(max(0, volume), 100) -@protocol.commands.add('single', state=protocol.boolean) +@protocol.commands.add('single', state=protocol.BOOL) def single(context, state): """ *musicpd.org, playback section:* diff --git a/tests/mpd/test_commands.py b/tests/mpd/test_commands.py index f0f04bb7..845796af 100644 --- a/tests/mpd/test_commands.py +++ b/tests/mpd/test_commands.py @@ -9,23 +9,49 @@ from mopidy.mpd import protocol class TestConverts(unittest.TestCase): def test_integer(self): - self.assertEqual(123, protocol.integer('123')) - self.assertEqual(-123, protocol.integer('-123')) - self.assertEqual(123, protocol.integer('+123')) - self.assertRaises(ValueError, protocol.integer, '3.14') - self.assertRaises(ValueError, protocol.integer, '') - self.assertRaises(ValueError, protocol.integer, 'abc') - self.assertRaises(ValueError, protocol.integer, '12 34') + self.assertEqual(123, protocol.INT('123')) + self.assertEqual(-123, protocol.INT('-123')) + self.assertEqual(123, protocol.INT('+123')) + self.assertRaises(ValueError, protocol.INT, '3.14') + self.assertRaises(ValueError, protocol.INT, '') + self.assertRaises(ValueError, protocol.INT, 'abc') + self.assertRaises(ValueError, protocol.INT, '12 34') + + def test_unsigned_integer(self): + self.assertEqual(123, protocol.UINT('123')) + self.assertRaises(ValueError, protocol.UINT, '-123') + self.assertRaises(ValueError, protocol.UINT, '+123') + self.assertRaises(ValueError, protocol.UINT, '3.14') + self.assertRaises(ValueError, protocol.UINT, '') + self.assertRaises(ValueError, protocol.UINT, 'abc') + self.assertRaises(ValueError, protocol.UINT, '12 34') def test_boolean(self): - self.assertEqual(True, protocol.boolean('1')) - self.assertEqual(False, protocol.boolean('0')) - self.assertRaises(ValueError, protocol.boolean, '3.14') - self.assertRaises(ValueError, protocol.boolean, '') - self.assertRaises(ValueError, protocol.boolean, 'true') - self.assertRaises(ValueError, protocol.boolean, 'false') - self.assertRaises(ValueError, protocol.boolean, 'abc') - self.assertRaises(ValueError, protocol.boolean, '12 34') + self.assertEqual(True, protocol.BOOL('1')) + self.assertEqual(False, protocol.BOOL('0')) + self.assertRaises(ValueError, protocol.BOOL, '3.14') + self.assertRaises(ValueError, protocol.BOOL, '') + self.assertRaises(ValueError, protocol.BOOL, 'true') + self.assertRaises(ValueError, protocol.BOOL, 'false') + self.assertRaises(ValueError, protocol.BOOL, 'abc') + self.assertRaises(ValueError, protocol.BOOL, '12 34') + + def test_range(self): + self.assertEqual(slice(1, 2), protocol.RANGE('1')) + self.assertEqual(slice(0, 1), protocol.RANGE('0')) + self.assertEqual(slice(0, None), protocol.RANGE('0:')) + self.assertEqual(slice(1, 3), protocol.RANGE('1:3')) + self.assertRaises(ValueError, protocol.RANGE, '3.14') + self.assertRaises(ValueError, protocol.RANGE, '1:abc') + self.assertRaises(ValueError, protocol.RANGE, 'abc:1') + self.assertRaises(ValueError, protocol.RANGE, '2:1') + self.assertRaises(ValueError, protocol.RANGE, '-1:2') + self.assertRaises(ValueError, protocol.RANGE, '1 : 2') + self.assertRaises(ValueError, protocol.RANGE, '') + self.assertRaises(ValueError, protocol.RANGE, 'true') + self.assertRaises(ValueError, protocol.RANGE, 'false') + self.assertRaises(ValueError, protocol.RANGE, 'abc') + self.assertRaises(ValueError, protocol.RANGE, '12 34') class TestCommands(unittest.TestCase):