From 6327a678749b751033e4d885380e941eb7a18cce Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 00:47:53 +0200 Subject: [PATCH 1/4] models: Make sure we really only add __weakref__ once --- mopidy/models.py | 8 +++++--- tests/models/test_models.py | 11 +++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 4b268474..477b87a7 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -1,6 +1,7 @@ from __future__ import absolute_import, unicode_literals import copy +import inspect import json import weakref @@ -145,11 +146,12 @@ class ImmutableObjectMeta(type): value._name = key attrs['_fields'] = fields - attrs['__slots__'] = fields.values() attrs['_instances'] = weakref.WeakValueDictionary() + attrs['__slots__'] = fields.values() - for base in bases: - if '__weakref__' in getattr(base, '__slots__', []): + anncestors = [b for base in bases for b in inspect.getmro(base)] + for anncestor in anncestors: + if '__weakref__' in getattr(anncestor, '__slots__', []): break else: attrs['__slots__'].append('__weakref__') diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 0407056c..27d02382 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -8,6 +8,17 @@ from mopidy.models import ( TlTrack, Track, model_json_decoder) +class InheritanecTest(unittest.TestCase): + + def test_weakref_and_slots_play_nice_in_subclass(self): + # Check that the following does not happen: + # TypeError: Error when calling the metaclass bases + # __weakref__ slot disallowed: either we already got one... + + class Foo(Track): + pass + + class CachingTest(unittest.TestCase): def test_same_instance(self): From 79d1862510331701308618001d8989e3b42f32af Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 01:07:18 +0200 Subject: [PATCH 2/4] models: Compare stream of items for models __eq__ Creating dictionaries for this is was just wasteful. --- mopidy/models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/models.py b/mopidy/models.py index 477b87a7..230c86cc 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, unicode_literals import copy import inspect +import itertools import json import weakref @@ -225,7 +226,8 @@ class ImmutableObject(object): def __eq__(self, other): if not isinstance(other, self.__class__): return False - return dict(self._items()) == dict(other._items()) + return all(a == b for a, b in itertools.izip_longest( + self._items(), other._items(), fillvalue=object())) def __ne__(self, other): return not self.__eq__(other) From 777a663896fcb937f2cf6987794242081b6e4808 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 11 Apr 2015 01:10:00 +0200 Subject: [PATCH 3/4] models: Take advantage of fact that our hash won't change This might just be pointless micro-optimization as I have _not_ measured. But it seemed silly to recursively hash everything in a model each time a hash is required. As we know the data can not change. --- mopidy/models.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 230c86cc..03f991ab 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -148,7 +148,7 @@ class ImmutableObjectMeta(type): attrs['_fields'] = fields attrs['_instances'] = weakref.WeakValueDictionary() - attrs['__slots__'] = fields.values() + attrs['__slots__'] = ['_hash'] + fields.values() anncestors = [b for base in bases for b in inspect.getmro(base)] for anncestor in anncestors: @@ -218,10 +218,12 @@ class ImmutableObject(object): } def __hash__(self): - hash_sum = 0 - for key, value in self._items(): - hash_sum += hash(key) + hash(value) - return hash_sum + if not hasattr(self, '_hash'): + hash_sum = 0 + for key, value in self._items(): + hash_sum += hash(key) + hash(value) + super(ImmutableObject, self).__setattr__('_hash', hash_sum) + return self._hash def __eq__(self, other): if not isinstance(other, self.__class__): @@ -267,6 +269,7 @@ class ImmutableObject(object): raise TypeError( 'copy() got an unexpected keyword argument "%s"' % key) super(ImmutableObject, other).__setattr__(key, value) + super(ImmutableObject, other).__delattr__('_hash') return self._instances.setdefault(weakref.ref(other), other) def serialize(self): From 20019edf2d904f7b58924e8ae04cb6c9756dd827 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 12 Apr 2015 16:03:51 +0200 Subject: [PATCH 4/4] models: Fix review comments --- mopidy/models.py | 5 ++--- tests/models/test_models.py | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 03f991ab..f4404fb8 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -150,9 +150,8 @@ class ImmutableObjectMeta(type): attrs['_instances'] = weakref.WeakValueDictionary() attrs['__slots__'] = ['_hash'] + fields.values() - anncestors = [b for base in bases for b in inspect.getmro(base)] - for anncestor in anncestors: - if '__weakref__' in getattr(anncestor, '__slots__', []): + for ancestor in [b for base in bases for b in inspect.getmro(base)]: + if '__weakref__' in getattr(ancestor, '__slots__', []): break else: attrs['__slots__'].append('__weakref__') diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 27d02382..c9c91ba1 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -8,7 +8,7 @@ from mopidy.models import ( TlTrack, Track, model_json_decoder) -class InheritanecTest(unittest.TestCase): +class InheritanceTest(unittest.TestCase): def test_weakref_and_slots_play_nice_in_subclass(self): # Check that the following does not happen: