Fix PersistentDict.update to accept keyword arguments.

Fixes #126.

Also optimize clear and popitem to not mark the object as changed unnecessarily.

CHANGES.rst

@@ -4,9 +4,21 @@
 4.5.2 (unreleased)
 ------------------
 
-- Fixed ``PersistentList`` to mark itself as changed after calling
+- Fix ``PersistentList`` to mark itself as changed after calling
   ``clear``. See `PR 115 <https://github.com/zopefoundation/persistent/pull/115/>`_.
 
+- Fix ``PersistentMapping.update`` to accept keyword arguments like
+  the native ``UserDict``. Previously, most uses of keyword arguments
+  resulted in ``TypeError``; in the undocumented and extremely
+  unlikely event of a single keyword argument called ``b`` that
+  happens to be a dictionary, the behaviour will change. Also adjust
+  the signatures of ``setdefault`` and ``pop`` to match the native
+  version.
+
+- Fix ``PersistentList.clear``, ``PersistentMapping.clear`` and
+  ``PersistentMapping.popitem`` to no longer mark the object as
+  changed if it was empty.
+
 4.5.1 (2019-11-06)
 ------------------
 

persistent/mapping.py

@@ -64,34 +64,68 @@ def __setitem__(self, key, v):
         self._p_changed = 1
 
     def clear(self):
+        """
+        Remove all data from this dictionary.
+
+        .. versionchanged:: 4.5.2
+           If there was nothing to remove, this object is no
+           longer marked as modified.
+        """
+        # Historically this method always marked ourself as changed,
+        # so if there was a _container it was persisted as data. We want
+        # to preserve that, even if we won't make any modifications otherwise.
+        needs_changed = '_container' in self.__dict__ or bool(self)
+        # Python 2 implements this by directly calling self.data.clear(),
+        # but Python 3 does so by repeatedly calling self.popitem()
         self.__super_clear()
+        if needs_changed:
+            self._p_changed = 1
+            assert self._p_changed
+
+    def update(self, *args, **kwargs):
+        """
+        D.update([E, ]**F) -> None.
+
+        .. versionchanged:: 4.5.2
+           Now accepts arbitrary keyword arguments. In the special case
+           of a keyword argument named ``b`` that is a dictionary,
+           the behaviour will change.
+        """
+        self.__super_update(*args, **kwargs)
         self._p_changed = 1
 
-    def update(self, b):
-        self.__super_update(b)
-        self._p_changed = 1
+    def setdefault(self, key, *args, **kwargs): # pylint:disable=arguments-differ
+        # (Python 3 and Python 2 have very different signatures.)
 
-    def setdefault(self, key, failobj=None):
         # We could inline all of UserDict's implementation into the
         # method here, but I'd rather not depend at all on the
         # implementation in UserDict (simple as it is).
-        if not key in self.data:
+        if key not in self.data:
             self._p_changed = 1
-        return self.__super_setdefault(key, failobj)
+        return self.__super_setdefault(key, *args, **kwargs)
 
-    def pop(self, key, *args):
+    def pop(self, key, *args, **kwargs): # pylint:disable=arguments-differ
+        # (Python 3 and Python 2 have very different signatures.)
         self._p_changed = 1
-        return self.__super_pop(key, *args)
+        return self.__super_pop(key, *args, **kwargs)
 
     def popitem(self):
+        """
+        Remove an item.
+
+        .. versionchanged:: 4.5.2
+           No longer marks this object as modified if it was empty
+           and an exception raised.
+        """
+        result = self.__super_popitem()
         self._p_changed = 1
-        return self.__super_popitem()
+        return result
 
     # Old implementations used _container rather than data.
     # Use a descriptor to provide data when we have _container instead
 
     @default
-    def data(self):
+    def data(self): # pylint:disable=method-hidden
         # We don't want to cause a write on read, so we're careful not to
         # do anything that would cause us to become marked as changed, however,
         # if we're modified, then the saved record will have data, not

persistent/tests/test_list.py

@@ -16,6 +16,8 @@
 
 import unittest
 
+from persistent.tests.utils import TrivialJar
+
 l0 = []
 l1 = [0]
 l2 = [0, 1]
@@ -30,17 +32,15 @@ def __len__(self):
     def __getitem__(self, i):
         return self.__data[i]
 
+
 class TestPList(unittest.TestCase):
 
     def _getTargetClass(self):
         from persistent.list import PersistentList
         return PersistentList
 
     def _makeJar(self):
-        class Jar(object):
-            def register(self, obj):
-                "no-op"
-        return Jar()
+        return TrivialJar()
 
     def _makeOne(self, *args):
         inst = self._getTargetClass()(*args)

persistent/tests/test_mapping.py

@@ -13,6 +13,8 @@
 ##############################################################################
 import unittest
 
+from persistent.tests.utils import TrivialJar
+
 # pylint:disable=blacklisted-name, protected-access
 
 class Test_default(unittest.TestCase):
@@ -53,8 +55,13 @@ def _getTargetClass(self):
         from persistent.mapping import PersistentMapping
         return PersistentMapping
 
-    def _makeOne(self, *args, **kw):
-        return self._getTargetClass()(*args, **kw)
+    def _makeJar(self):
+        return TrivialJar()
+
+    def _makeOne(self, *args, **kwargs):
+        inst = self._getTargetClass()(*args, **kwargs)
+        inst._p_jar = self._makeJar()
+        return inst
 
     def test_volatile_attributes_not_persisted(self):
         # http://www.zope.org/Collectors/Zope/2052
@@ -66,7 +73,6 @@ def test_volatile_attributes_not_persisted(self):
         self.assertFalse('_v_baz' in state)
 
     def testTheWorld(self):
-        from persistent._compat import PYTHON2
         # Test constructors
         l0 = {}
         l1 = {0:0}
@@ -83,6 +89,7 @@ def testTheWorld(self):
 
         class OtherMapping(dict):
             def __init__(self, initmapping):
+                dict.__init__(self)
                 self.__data = initmapping
             def items(self):
                 raise AssertionError("Not called")
@@ -221,6 +228,59 @@ def test___repr___converts_legacy_container_attr(self):
         self.assertEqual(pm.__dict__, {'data': {'a': 1}})
         self.assertEqual(pm.__getstate__(), {'data': {'a': 1}})
 
+    def test_update_keywords(self):
+        # Prior to https://github.com/zopefoundation/persistent/issues/126,
+        # PersistentMapping didn't accept keyword arguments to update as
+        # the builtin dict and the UserDict do.
+        # Here we make sure it does. We use some names that have been
+        # seen to be special in signatures as well to make sure that
+        # we don't interpret them incorrectly.
+        pm = self._makeOne()
+        # Our older implementation was ``def update(self, b)``, so ``b``
+        # is potentially a keyword argument in the wild; the behaviour in that
+        # corner case has changed.
+        pm.update(b={'a': 42})
+        self.assertEqual(pm, {'b': {'a': 42}})
+
+        pm = self._makeOne()
+        # Our previous implementation would explode with a TypeError
+        pm.update(b=42)
+        self.assertEqual(pm, {'b': 42})
+
+        pm = self._makeOne()
+        # ``other`` shows up in a Python 3 signature.
+        pm.update(other=42)
+        self.assertEqual(pm, {'other': 42})
+        pm = self._makeOne()
+        pm.update(other={'a': 42})
+        self.assertEqual(pm, {'other': {'a': 42}})
+
+        pm = self._makeOne()
+        pm.update(a=1, b=2)
+        self.assertEqual(pm, {'a': 1, 'b': 2})
+
+    def test_clear_nonempty(self):
+        pm = self._makeOne({'a': 42})
+        self.assertFalse(pm._p_changed)
+        pm.clear()
+        self.assertTrue(pm._p_changed)
+
+    def test_clear_empty(self):
+        pm = self._makeOne()
+        self.assertFalse(pm._p_changed)
+        pm.clear()
+        self.assertFalse(pm._p_changed)
+
+    def test_clear_empty_legacy_container(self):
+        pm = self._makeOne()
+        pm.__dict__['_container'] = pm.__dict__.pop('data')
+        self.assertFalse(pm._p_changed)
+        pm.clear()
+        # Migration happened
+        self.assertIn('data', pm.__dict__)
+        # and we are marked as changed.
+        self.assertTrue(pm._p_changed)
+
 
 class Test_legacy_PersistentDict(unittest.TestCase):
 
@@ -230,7 +290,7 @@ def _getTargetClass(self):
 
     def test_PD_is_alias_to_PM(self):
         from persistent.mapping import PersistentMapping
-        self.assertTrue(self._getTargetClass() is PersistentMapping)
+        self.assertIs(self._getTargetClass(), PersistentMapping)
 
 
 def test_suite():

persistent/tests/utils.py

@@ -1,4 +1,13 @@
 
+class TrivialJar(object):
+    """
+    Jar that only supports registering objects so ``_p_changed``
+    can be tested.
+    """
+
+    def register(self, ob):
+        """Does nothing"""
+
 class ResettingJar(object):
     """Testing stub for _p_jar attribute.
     """