Merge lp:~mandel/desktopcouch/fix_bug_519873 into lp:desktopcouch

[Manuel de la Peña]

Make list-like MergableList objects behave more like lists by adding pop() and remove() methods. (LP: #519873)

[Manuel de la Peña (mandel)]

As explained in bug 519873 the remove and pop methods are missing. Although the optimal solution for this kind of problem would be implement a Bimap for the MergeableList this is not feseable because python lists and dicts are not hashable. This solution provides a O(n*m) performance where n is the number of objects in the mergeable list and m is the size of the possible inner lists.

[Chad Miller (cmiller)]

"for index, current_value in enumerate(self):" is preferable to "index = 0", then "for current_value in..." and "index += 1" inside.

Also, I'm not too enthusiastic about restricting pop/remove on the last item. It's not a problem for lists to remove the last item. Perhaps we should destroy the container that must not be empty instead, when it reaches zero entries. Or, change the container so that it may be empty? (I do not understand the empty restriction yet.)

review: Needs Fixing

[Manuel de la Peña (mandel)]

The empty restriction is coming from the RecordData class that does not allow to add empty lists so we do not allow to add them and then delete all its contents. I know there is a bug files regarding this (https://bugs.launchpad.net/desktopcouch/+bug/510232) maybe fixing that bug and then merging this branch without the last pop and remove check is a better idea.

[Eric Casteleijn (thisfred)]

I suggested a solution to https://bugs.launchpad.net/desktopcouch/+bug/510232 on that bug's page, which I agree should probably be fixed first, which would make a nicer solution possible here. (Also not all lists would be mergeable lists, so any performance hit for very large lists would be circumventable by not making them mergeable lists.)

[Chad Miller (cmiller)]

review: Approve

[John O'Brien (jdobrien)]

The code looks good but I get unrelated errors.

======================================================================
FAIL: test_get_my_host_unique_id
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/internet/defer.py", line 106, in maybeDeferred
    result = f(*args, **kw)
  File "/usr/lib/python2.6/dist-packages/twisted/internet/utils.py", line 191, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/john/canonical/ubuntuone/desktopcouch/fix_bug_519873/desktopcouch/pair/tests/test_couchdb_io.py", line 134, in test_get_my_host_unique_id
    self.assertEquals(got, again)
  File "/usr/lib/python2.6/dist-packages/twisted/trial/unittest.py", line 262, in failUnlessEqual
    % (msg, pformat(first), pformat(second)))
FailTest: not equal:
a = ['a0624880-6e12-4794-8010-32a973c43c8f']
b = ['5946ee98-d83f-479e-ba0f-3f88bfa11a6c']

review: Approve

[Manuel de la Peña (mandel)]

Interesting, are you running Lucid? I ran the tests on Karmic and everything was find there.. Let me know so I pump my OS to try and find the error.

> The code looks good but I get unrelated errors.
>
>
> ======================================================================
> FAIL: test_get_my_host_unique_id
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.6/dist-packages/twisted/internet/defer.py", line 106,
> in maybeDeferred
> result = f(*args, **kw)
> File "/usr/lib/python2.6/dist-packages/twisted/internet/utils.py", line 191,
> in runWithWarningsSuppressed
> result = f(*a, **kw)
> File "/home/john/canonical/ubuntuone/desktopcouch/fix_bug_519873/desktopcouc
> h/pair/tests/test_couchdb_io.py", line 134, in test_get_my_host_unique_id
> self.assertEquals(got, again)
> File "/usr/lib/python2.6/dist-packages/twisted/trial/unittest.py", line 262,
> in failUnlessEqual
> % (msg, pformat(first), pformat(second)))
> FailTest: not equal:
> a = ['a0624880-6e12-4794-8010-32a973c43c8f']
> b = ['5946ee98-d83f-479e-ba0f-3f88bfa11a6c']

[Chad Miller (cmiller)]

Attempt to merge lp:~mandel/desktopcouch/fix_bug_519873 into lp:desktopcouch failed due to merge conflicts:

text conflict in bin/desktopcouch-stop

[Chad Miller (cmiller)]

Attempt to merge lp:~mandel/desktopcouch/fix_bug_519873 into lp:desktopcouch failed due to merge conflicts:

text conflict in bin/desktopcouch-stop

[lp:~mandel/desktopcouch/fix_bug_519873]

125. By Manuel de la Peña on 2010-02-25

Merged with current trunk

[Manuel de la Peña (mandel)]

The merge should be possible now. As a side note since I have been using Lucid I get the fails that John reported.
> Attempt to merge lp:~mandel/desktopcouch/fix_bug_519873 into lp:desktopcouch
> failed due to merge conflicts:
>
> text conflict in bin/desktopcouch-stop

[Chad Miller (cmiller)]

Tarmac has no memory. When it marks a failed merge as Needs Review, it doesn't try again until someone marks the branch Approved again.

=== modified file 'bin/desktopcouch-stop' (properties changed: +x to -x)
=== modified file 'desktopcouch/records/record.py'
--- desktopcouch/records/record.py	2010-01-22 20:52:35 +0000
+++ desktopcouch/records/record.py	2010-02-25 22:39:13 +0000
@@ -198,6 +198,11 @@
     def __contains__(self, key):
         return key in self._data
 
+    def __cmp__(self, value):
+        if isinstance(value, RecordDict):
+            return cmp(self._data, value._data)
+        return -1
+        
     def get(self, key, default=None):
         """Override get method."""
         if not key in self._data:
@@ -271,6 +276,24 @@
             value = value._data
         return value in self._data.values()
 
+    def __cmp__(self, value):
+        """
+        Implement the compare to be able to compare with other mergeable
+        lists, tuples and lists.
+        """
+        if (hasattr(value, "__iter__") 
+            and hasattr(value, "__getitem__")
+            and hasattr(value, "__len__")):
+            # we should have the same value in the same order
+            length  = len(value)
+            if len(self) == length:
+                for index, current_value in enumerate(self):
+                    cmp_value = cmp(self[index], value[index])
+                    if cmp_value != 0:
+                        return cmp_value
+                return 0
+        return -1
+        
     def _get_ordered_keys(self):
         """Get list of uuid keys ordered by 'order' property or uuid key."""
         result = []
@@ -299,6 +322,26 @@
             new_uuid)
         super(MergeableList, self).__setitem__(new_uuid, value)
 
+    def remove(self, value):
+        if len(self) == 1:
+            raise ValueError("MergeableList cannot be empty.")
+        index = 0
+        for current_value in self:
+            # important! use the data in self first 'cause mergeable lists
+            # can be compared with list and tuples but no the other way around
+            if cmp(current_value, value) == 0:
+                del self[index]
+                return
+            index += 1
+        raise ValueError("list.remove(x): x not in list")
+        
+    def pop(self, index):
+        if len(self) == 1:
+            raise ValueError("MergeableList cannot be empty.")
+        value = self[index]
+        del self[index]
+        return value
+        
     def index(self, key):
         """Get value by index."""
         return self.__getitem__(key)

=== modified file 'desktopcouch/records/tests/test_record.py'
--- desktopcouch/records/tests/test_record.py	2010-01-29 22:14:25 +0000
+++ desktopcouch/records/tests/test_record.py	2010-02-25 22:39:13 +0000
@@ -177,7 +177,69 @@
         self.record["subfield_uuid"].append(value)
         self.assertEqual(3, len(self.record["subfield_uuid"]))
         self.assertEqual("value32", self.record["subfield_uuid"][-1]["field32"])
+        
+    def test_mergeable_list_remove(self):
+        """Test remove a normal value"""
+        value = "string"
+        self.record["subfield_uuid"].append(value)
+        self.record["subfield_uuid"].remove(value)
+        self.assertFalse(value in self.record["subfield_uuid"])
+        
+    def test_mergeable_list_remove_record_dict(self):
+        """Test remove a RecordDict value"""
+        value = RecordDict({
+            "field31": "value31",
+            "field32": "value32"})
+        self.record["subfield_uuid"].append(value)
+        self.record["subfield_uuid"].remove(value)
+        self.assertFalse(value in self.record["subfield_uuid"])
 
+    def test_mergeable_list_remove_list(self):
+        """Test list removal"""
+        value = [1, 2, 3, 4, 5]
+        self.record["subfield_uuid"].append(value)
+        self.record["subfield_uuid"].remove(value)
+        self.assertFalse(value in self.record["subfield_uuid"])
+        
+    def test_mergeable_list_remove_tuple(self):
+        """Test tuple removal"""
+        value = (1, 2, 3, 4, 5)
+        self.record["subfield_uuid"].append(value)
+        self.record["subfield_uuid"].remove(value)
+        self.assertFalse(value in self.record["subfield_uuid"])
+        
+    def test_mergeable_list_remove_missing(self):
+        """Test missing data rmeoval"""
+        self.assertRaises(ValueError, self.record["subfield_uuid"].remove, 
+            "missing_data")
+    def test_mergeable_list_remove_last(self):
+        """Test that exception is raised when removing last item."""
+        self.record["subfield_uuid"] = [1]
+        self.assertRaises(ValueError, self.record["subfield_uuid"].remove, 1)
+        
+    def test_mergeable_list_pop_correct_index(self):
+        """Test the pop method when working with a correct index."""
+        value = [1, 2, 3, 4, 5]
+        self.record["subfield_uuid"] = value
+        # test with negative index
+        poped_value = self.record["subfield_uuid"].pop(-2)
+        self.assertEqual(value[-2], poped_value)
+        # test with positive index
+        poped_value = self.record["subfield_uuid"].pop(1)
+        self.assertEqual(value[1], poped_value)
+        
+    def test_mergeable_list_pop_wrong_index(self):
+        """Test pop when index is out or range."""
+        value = [1, 2, 3, 4, 5]
+        self.record["subfield_uuid"] = value
+        self.assertRaises(IndexError, self.record["subfield_uuid"].pop, 
+            len(value)*2)
+    
+    def test_mergeable_list_pop_last(self):
+        """Test that exception is raised when poping last item"""
+        self.record["subfield_uuid"] = [1]
+        self.assertRaises(ValueError, self.record["subfield_uuid"].pop, 0)
+        
     def test_tuple(self):
         """Test assigning tuples to a key results in mergeable lists."""
         rec = Record({'record_type': 'http://fnord.org/smorgasbord'})