Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #21840 -- Moved dunder methods from SimpleLazyObject to LazyObject... #2221

Merged
merged 2 commits into from Mar 20, 2014

Conversation

Projects
None yet
5 participants
Member

bmispelon commented Jan 29, 2014

....

@timgraham timgraham commented on an outdated diff Feb 4, 2014

django/utils/functional.py
+ # Dictionary methods support
+ __getitem__ = new_method_proxy(operator.getitem)
+ __setitem__ = new_method_proxy(operator.setitem)
+ __delitem__ = new_method_proxy(operator.delitem)
+
+ __len__ = new_method_proxy(len)
+ __contains__ = new_method_proxy(operator.contains)
+
+
+# Workaround for http://bugs.python.org/issue12370
+_super = super
+
+
+class SimpleLazyObject(LazyObject):
+ """
+ A lazy object initialised from any function.
@timgraham

timgraham Feb 4, 2014

Owner

initialized*

@timgraham timgraham commented on the diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
@@ -0,0 +1,254 @@
+from __future__ import unicode_literals
+
+import copy
@timgraham

timgraham Mar 12, 2014

Owner

alphabetize imports

@bmispelon

bmispelon Mar 12, 2014

Member

Good catch, thanks.

@timgraham timgraham commented on an outdated diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
@@ -0,0 +1,254 @@
+from __future__ import unicode_literals
+
+import copy
+from unittest import TestCase
+import pickle
+import sys
+
+from django.utils.functional import LazyObject, SimpleLazyObject, empty
+from django.utils import six
@timgraham

timgraham Mar 12, 2014

Owner

I'd order utils before utils.functional

@timgraham timgraham commented on an outdated diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+from unittest import TestCase
+import pickle
+import sys
+
+from django.utils.functional import LazyObject, SimpleLazyObject, empty
+from django.utils import six
+
+
+class Foo(object):
+ """
+ A simple class with just one attribute.
+ """
+ foo = 'bar'
+
+ def __eq__(self, other):
+ return self.foo== other.foo
@timgraham

timgraham Mar 12, 2014

Owner

spacing around ==

@timgraham timgraham commented on the diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+ self.assertEqual(obj2, [1, 2, 3])
+
+ def test_deepcopy_no_evaluation(self):
+ # copying doesn't force evaluation
+
+ l = [1, 2, 3]
+
+ obj = self.lazy_wrap(l)
+ obj2 = copy.deepcopy(obj)
+
+ # Copying shouldn't force evaluation
+ self.assertIs(obj._wrapped, empty)
+ self.assertIs(obj2._wrapped, empty)
+
+
+class SimpleLazyObjectTestCase(LazyObjectTestCase):
@timgraham

timgraham Mar 12, 2014

Owner

what's the purpose of inheriting from LazyObjectTestCase? I believe that causes all its tests to be run twice. Better to write a base test class with no tests and inherit from that I think.

@bmispelon

bmispelon Mar 12, 2014

Member

SimpleLazyObjectTestCase redefines the lazy_wrap() method on which all test depends.

This is useful to make sure that the special "dunder" methods we defined on LazyObject still work with SimpleLazyObject.

@timgraham

timgraham Mar 12, 2014

Owner

Makes sense, would be helpful to add a comment about this.

@bmispelon

bmispelon Mar 12, 2014

Member

You're right. Done.

@timgraham timgraham commented on an outdated diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+ self.assertEqual(repr(obj), '<SimpleLazyObject: 42>')
+
+ def test_trace(self):
+ # See ticket #19456
+ old_trace_func = sys.gettrace()
+ try:
+ def trace_func(frame, event, arg):
+ frame.f_locals['self'].__class__
+ if old_trace_func is not None:
+ old_trace_func(frame, event, arg)
+ sys.settrace(trace_func)
+ self.lazy_wrap(None)
+ finally:
+ sys.settrace(old_trace_func)
+
+ # Old tests that used to be in test_simplelazyobject.py
@timgraham

timgraham Mar 12, 2014

Owner

extra newline below + don't know that the comment adds much value

@shaib shaib commented on the diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+ class AdHocLazyObject(LazyObject):
+ def _setup(self):
+ self._wrapped = wrapped_object
+
+ return AdHocLazyObject()
+
+ def test_getattr(self):
+ obj = self.lazy_wrap(Foo())
+ self.assertEqual(obj.foo, 'bar')
+
+ def test_setattr(self):
+ obj = self.lazy_wrap(Foo())
+ obj.foo = 'BAR'
+ obj.bar = 'baz'
+ self.assertEqual(obj.foo, 'BAR')
+ self.assertEqual(obj.bar, 'baz')
@shaib

shaib Mar 12, 2014

Member

I'd add a test where the attributes are added in reversed order.

@bmispelon

bmispelon Mar 12, 2014

Member

Good idea. Done.

@shaib shaib commented on an outdated diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+ self.assertEqual(obj1, 'foo')
+ self.assertEqual(obj1, obj3)
+ self.assertNotEqual(obj1, obj2)
+ self.assertNotEqual(obj1, 'bar')
+
+ def test_bytes(self):
+ obj = self.lazy_wrap(b'foo')
+ self.assertEqual(bytes(obj), b'foo')
+
+ def test_text(self):
+ obj = self.lazy_wrap('foo')
+ self.assertEqual(six.text_type(obj), 'foo')
+
+ def test_bool(self):
+ # Refs #21840
+ for f in [False, 0, (), {}, []]:
@shaib

shaib Mar 12, 2014

Member

None? set()?

@shaib shaib and 1 other commented on an outdated diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+
+ def test_hash(self):
+ obj = self.lazy_wrap('foo')
+ d = {}
+ d[obj] = 'bar'
+ self.assertIn('foo', d)
+ self.assertEqual(d['foo'], 'bar')
+
+ def test_contains(self):
+ test_data = [
+ ('c', 'abcde'),
+ (2, [1, 2, 3]),
+ ('a', {'a': 1, 'b': 2, 'c': 3})
+ ]
+ for needle, haystack in test_data:
+ self.assertTrue(needle, self.lazy_wrap(haystack))
@shaib

shaib Mar 12, 2014

Member

That should be assertIn, no?

@shaib

shaib Mar 12, 2014

Member

Also, I'd add

for needle, haystack in test_data:
    self.assertIn(self.lazy_wrap(needle), haystack)
@bmispelon

bmispelon Mar 12, 2014

Member

Thanks for catching that.

I added the suggested test but in a slightly different way since it fails when the haystack is a string and the needle a lazy object (this behavior was present before so it's not a regression).

@shaib shaib commented on the diff Mar 12, 2014

tests/utils_tests/test_lazyobject.py
+ self.assertTrue('one' in lazydict)
+ self.assertFalse('two' in lazydict)
+ self.assertEqual(len(lazydict), 1)
+ del lazydict['one']
+ with self.assertRaises(KeyError):
+ lazydict['one']
+
+ def test_list_set(self):
+ lazy_list = SimpleLazyObject(lambda: [1, 2, 3, 4, 5])
+ lazy_set = SimpleLazyObject(lambda: set([1, 2, 3, 4]))
+ self.assertTrue(1 in lazy_list)
+ self.assertTrue(1 in lazy_set)
+ self.assertFalse(6 in lazy_list)
+ self.assertFalse(6 in lazy_set)
+ self.assertEqual(len(lazy_list), 5)
+ self.assertEqual(len(lazy_set), 4)
@shaib

shaib Mar 12, 2014

Member

Add tests for iteration? something like assertTrue(all(x>0 for x in lazy_list) and any(x>0 for x in lazy_list))

@bmispelon

bmispelon Mar 12, 2014

Member

Good idea. I added an extra testcase method on the parent class.

It's actually interesting to test since LazyObject doesn't implement __iter__ so iteration works with __getitem__.

@loic loic commented on an outdated diff Mar 13, 2014

django/utils/functional.py
- # Need to pretend to be the wrapped class, for the sake of objects that
- # care about this (especially in equality tests)
- __class__ = property(new_method_proxy(operator.attrgetter("__class__")))
- __eq__ = new_method_proxy(operator.eq)
- __ne__ = new_method_proxy(operator.ne)
- __hash__ = new_method_proxy(hash)
- __bool__ = new_method_proxy(bool) # Python 3
- __nonzero__ = __bool__ # Python 2
+ def __deepcopy__(self, memo):
+ if self._wrapped is empty:
+ # We have to use type(self), not self.__class__, because the
@loic

loic Mar 13, 2014

Member

We don't seem to use type(self) nor self.__class__, does this comment apply?

bmispelon added some commits Jan 29, 2014

Fixed #21840 -- Moved dunder methods from SimpleLazyObject to LazyObj…
…ect.

This commit also added tests for LazyObject and refactored
the testsuite of SimpleLazyObject so that it can share
test cases with LazyObject.
Simplified implementation of collectstatic command.
Since d2e242d16c6dde6f4736086fb38057424bed3edb made isinstance()
calls work correctly on LazyObject, we can simplify the
implementation of is_local_storage added in
7e27885.

andrewgodwin added a commit that referenced this pull request Mar 20, 2014

Merge pull request #2221 from bmispelon/LazyObject-refactor
Fixed #21840 -- Moved dunder methods from SimpleLazyObject to LazyObject...

@andrewgodwin andrewgodwin merged commit 356f064 into django:master Mar 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment