diff --git a/docs/api/models.rst b/docs/api/models.rst index 7192c284..d2d8ec0a 100644 --- a/docs/api/models.rst +++ b/docs/api/models.rst @@ -8,7 +8,7 @@ and immutable. In other words, they can only be set through the class constructor during instance creation. If you want to modify a model, use the -:meth:`~mopidy.models.ImmutableObject.copy` method. It accepts keyword +:meth:`~mopidy.models.ImmutableObject.replace` method. It accepts keyword arguments for the parts of the model you want to change, and copies the rest of the data from the model you call it on. Example:: @@ -16,7 +16,7 @@ the data from the model you call it on. Example:: >>> track1 = Track(name='Christmas Carol', length=171) >>> track1 Track(artists=[], length=171, name='Christmas Carol') - >>> track2 = track1.copy(length=37) + >>> track2 = track1.replace(length=37) >>> track2 Track(artists=[], length=37, name='Christmas Carol') >>> track1 @@ -75,6 +75,7 @@ Data model helpers ================== .. autoclass:: mopidy.models.ImmutableObject + :members: .. autoclass:: mopidy.models.Field diff --git a/mopidy/models.py b/mopidy/models.py index 97b524ea..bd449a89 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -4,6 +4,8 @@ import copy import json import weakref +from mopidy.utils import deprecation + # TODO: split into base models, serialization and fields? @@ -166,6 +168,10 @@ class ImmutableObject(object): constructor. Fields should be :class:`Field` instances to ensure type safety in our models. + Note that since these models can not be changed, we heavily memoize them + to save memory. So constructing a class with the same arguments twice will + give you the same instance twice. + :param kwargs: kwargs to set as fields on the object :type kwargs: any """ @@ -224,23 +230,35 @@ class ImmutableObject(object): def copy(self, **values): """ - Copy the model with ``field`` updated to new value. + .. deprecated:: 1.1 + Use :meth:`replace` instead. Note that we no longer return copies. + """ + deprecation.warn('model.immutable.copy') + return self.replace(**values) + + def replace(self, **kwargs): + """ + Replace the fields in the model and return a new instance Examples:: # Returns a track with a new name - Track(name='foo').copy(name='bar') + Track(name='foo').replace(name='bar') # Return an album with a new number of tracks - Album(num_tracks=2).copy(num_tracks=5) + Album(num_tracks=2).replace(num_tracks=5) - :param values: the model fields to modify - :type values: dict - :rtype: new instance of the model being copied + Note that internally we memoize heavily to keep memory usage down given + our overly repetitive data structures. So you might get an existing + instance if it contains the same values. + + :param kwargs: kwargs to set as fields on the object + :type kwargs: any + :rtype: instance of the model with replaced fields """ - if not values: + if not kwargs: return self other = copy.copy(self) - for key, value in values.items(): + for key, value in kwargs.items(): if key not in self._fields: raise TypeError( 'copy() got an unexpected keyword argument "%s"' % key) diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index 57042347..4f26b41b 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -40,6 +40,9 @@ _MESSAGES = { 'tracklist.add() "tracks" argument is deprecated', 'core.tracklist.add:uri_arg': 'tracklist.add() "uri" argument is deprecated', + + 'models.immutable.copy': + 'ImmutableObject.copy() is deprecated, use ImmutableObject.replace()', } diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 90701b14..0407056c 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -19,58 +19,58 @@ class CachingTest(unittest.TestCase): def test_different_instance_with_different_values(self): self.assertIsNot(Track(uri='test1'), Track(uri='test2')) - def test_different_instance_with_copy(self): + def test_different_instance_with_replace(self): t = Track(uri='test1') - self.assertIsNot(t, t.copy(uri='test2')) + self.assertIsNot(t, t.replace(uri='test2')) -class GenericCopyTest(unittest.TestCase): +class GenericReplaceTest(unittest.TestCase): def compare(self, orig, other): self.assertEqual(orig, other) self.assertEqual(id(orig), id(other)) - def test_copying_track(self): + def test_replace_track(self): track = Track() - self.compare(track, track.copy()) + self.compare(track, track.replace()) - def test_copying_artist(self): + def test_replace_artist(self): artist = Artist() - self.compare(artist, artist.copy()) + self.compare(artist, artist.replace()) - def test_copying_album(self): + def test_replace_album(self): album = Album() - self.compare(album, album.copy()) + self.compare(album, album.replace()) - def test_copying_playlist(self): + def test_replace_playlist(self): playlist = Playlist() - self.compare(playlist, playlist.copy()) + self.compare(playlist, playlist.replace()) - def test_copying_track_with_basic_values(self): + def test_replace_track_with_basic_values(self): track = Track(name='foo', uri='bar') - copy = track.copy(name='baz') - self.assertEqual('baz', copy.name) - self.assertEqual('bar', copy.uri) + other = track.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) - def test_copying_track_with_missing_values(self): + def test_replace_track_with_missing_values(self): track = Track(uri='bar') - copy = track.copy(name='baz') - self.assertEqual('baz', copy.name) - self.assertEqual('bar', copy.uri) + other = track.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) - def test_copying_track_with_private_internal_value(self): + def test_replace_track_with_private_internal_value(self): artist1 = Artist(name='foo') artist2 = Artist(name='bar') track = Track(artists=[artist1]) - copy = track.copy(artists=[artist2]) - self.assertIn(artist2, copy.artists) + other = track.replace(artists=[artist2]) + self.assertIn(artist2, other.artists) - def test_copying_track_with_invalid_key(self): + def test_replace_track_with_invalid_key(self): with self.assertRaises(TypeError): - Track().copy(invalid_key=True) + Track().replace(invalid_key=True) - def test_copying_track_to_remove(self): - track = Track(name='foo').copy(name=None) + def test_replace_track_to_remove(self): + track = Track(name='foo').replace(name=None) self.assertFalse(hasattr(track, '_name')) @@ -209,7 +209,7 @@ class ArtistTest(unittest.TestCase): def test_invalid_kwarg_with_name_matching_method(self): with self.assertRaises(TypeError): - Artist(copy='baz') + Artist(replace='baz') with self.assertRaises(TypeError): Artist(serialize='baz') @@ -816,9 +816,9 @@ class TrackTest(unittest.TestCase): self.assertEqual(track1, track2) self.assertEqual(hash(track1), hash(track2)) - def test_copy_can_reset_to_default_value(self): + def test_replace_can_reset_to_default_value(self): track1 = Track(name='name1') - track2 = Track(name='name1', album=Album()).copy(album=None) + track2 = Track(name='name1', album=Album()).replace(album=None) self.assertEqual(track1, track2) self.assertEqual(hash(track1), hash(track2)) @@ -943,7 +943,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(uri='another uri') + new_playlist = playlist.replace(uri='another uri') self.assertEqual(new_playlist.uri, 'another uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), tracks) @@ -955,7 +955,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(name='another name') + new_playlist = playlist.replace(name='another name') self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'another name') self.assertEqual(list(new_playlist.tracks), tracks) @@ -968,7 +968,7 @@ class PlaylistTest(unittest.TestCase): uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) new_tracks = [Track(), Track()] - new_playlist = playlist.copy(tracks=new_tracks) + new_playlist = playlist.replace(tracks=new_tracks) self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), new_tracks) @@ -981,7 +981,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(last_modified=new_last_modified) + new_playlist = playlist.replace(last_modified=new_last_modified) self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), tracks)