desktopcouch

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

  1. fix_bug_519873
  2. Merge into trunk
Proposed by Manuel de la Peña
Status: Merged
Approved by: Chad Miller
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~mandel/desktopcouch/fix_bug_519873
Merge into: lp:desktopcouch
Diff against target: 142 lines (+105/-0)
2 files modified
desktopcouch/records/record.py (+43/-0)
desktopcouch/records/tests/test_record.py (+62/-0)
To merge this branch: bzr merge lp:~mandel/desktopcouch/fix_bug_519873
Reviewer Review Type Date Requested Status
John O'Brien (community) Approve
Chad Miller (community) Approve
Review via email: mp+19018@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Manuel de la Peña (mandel) wrote :

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.

Revision history for this message
Chad Miller (cmiller) wrote :

"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
Revision history for this message
Manuel de la Peña (mandel) wrote :

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.

Revision history for this message
Eric Casteleijn (thisfred) wrote :

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.)

Revision history for this message
Chad Miller (cmiller) :
review: Approve
Revision history for this message
John O'Brien (jdobrien) wrote :

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
Revision history for this message
Manuel de la Peña (mandel) wrote :

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']

Revision history for this message
Chad Miller (cmiller) wrote :

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

text conflict in bin/desktopcouch-stop

Revision history for this message
Chad Miller (cmiller) wrote :

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 updated
125. By Manuel de la Peña

Merged with current trunk

Revision history for this message
Manuel de la Peña (mandel) wrote :

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

Revision history for this message
Chad Miller (cmiller) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/desktopcouch-stop' (properties changed: +x to -x)
2=== modified file 'desktopcouch/records/record.py'
3--- desktopcouch/records/record.py 2010-01-22 20:52:35 +0000
4+++ desktopcouch/records/record.py 2010-02-25 22:39:13 +0000
5@@ -198,6 +198,11 @@
6 def __contains__(self, key):
7 return key in self._data
8
9+ def __cmp__(self, value):
10+ if isinstance(value, RecordDict):
11+ return cmp(self._data, value._data)
12+ return -1
13+
14 def get(self, key, default=None):
15 """Override get method."""
16 if not key in self._data:
17@@ -271,6 +276,24 @@
18 value = value._data
19 return value in self._data.values()
20
21+ def __cmp__(self, value):
22+ """
23+ Implement the compare to be able to compare with other mergeable
24+ lists, tuples and lists.
25+ """
26+ if (hasattr(value, "__iter__")
27+ and hasattr(value, "__getitem__")
28+ and hasattr(value, "__len__")):
29+ # we should have the same value in the same order
30+ length = len(value)
31+ if len(self) == length:
32+ for index, current_value in enumerate(self):
33+ cmp_value = cmp(self[index], value[index])
34+ if cmp_value != 0:
35+ return cmp_value
36+ return 0
37+ return -1
38+
39 def _get_ordered_keys(self):
40 """Get list of uuid keys ordered by 'order' property or uuid key."""
41 result = []
42@@ -299,6 +322,26 @@
43 new_uuid)
44 super(MergeableList, self).__setitem__(new_uuid, value)
45
46+ def remove(self, value):
47+ if len(self) == 1:
48+ raise ValueError("MergeableList cannot be empty.")
49+ index = 0
50+ for current_value in self:
51+ # important! use the data in self first 'cause mergeable lists
52+ # can be compared with list and tuples but no the other way around
53+ if cmp(current_value, value) == 0:
54+ del self[index]
55+ return
56+ index += 1
57+ raise ValueError("list.remove(x): x not in list")
58+
59+ def pop(self, index):
60+ if len(self) == 1:
61+ raise ValueError("MergeableList cannot be empty.")
62+ value = self[index]
63+ del self[index]
64+ return value
65+
66 def index(self, key):
67 """Get value by index."""
68 return self.__getitem__(key)
69
70=== modified file 'desktopcouch/records/tests/test_record.py'
71--- desktopcouch/records/tests/test_record.py 2010-01-29 22:14:25 +0000
72+++ desktopcouch/records/tests/test_record.py 2010-02-25 22:39:13 +0000
73@@ -177,7 +177,69 @@
74 self.record["subfield_uuid"].append(value)
75 self.assertEqual(3, len(self.record["subfield_uuid"]))
76 self.assertEqual("value32", self.record["subfield_uuid"][-1]["field32"])
77+
78+ def test_mergeable_list_remove(self):
79+ """Test remove a normal value"""
80+ value = "string"
81+ self.record["subfield_uuid"].append(value)
82+ self.record["subfield_uuid"].remove(value)
83+ self.assertFalse(value in self.record["subfield_uuid"])
84+
85+ def test_mergeable_list_remove_record_dict(self):
86+ """Test remove a RecordDict value"""
87+ value = RecordDict({
88+ "field31": "value31",
89+ "field32": "value32"})
90+ self.record["subfield_uuid"].append(value)
91+ self.record["subfield_uuid"].remove(value)
92+ self.assertFalse(value in self.record["subfield_uuid"])
93
94+ def test_mergeable_list_remove_list(self):
95+ """Test list removal"""
96+ value = [1, 2, 3, 4, 5]
97+ self.record["subfield_uuid"].append(value)
98+ self.record["subfield_uuid"].remove(value)
99+ self.assertFalse(value in self.record["subfield_uuid"])
100+
101+ def test_mergeable_list_remove_tuple(self):
102+ """Test tuple removal"""
103+ value = (1, 2, 3, 4, 5)
104+ self.record["subfield_uuid"].append(value)
105+ self.record["subfield_uuid"].remove(value)
106+ self.assertFalse(value in self.record["subfield_uuid"])
107+
108+ def test_mergeable_list_remove_missing(self):
109+ """Test missing data rmeoval"""
110+ self.assertRaises(ValueError, self.record["subfield_uuid"].remove,
111+ "missing_data")
112+ def test_mergeable_list_remove_last(self):
113+ """Test that exception is raised when removing last item."""
114+ self.record["subfield_uuid"] = [1]
115+ self.assertRaises(ValueError, self.record["subfield_uuid"].remove, 1)
116+
117+ def test_mergeable_list_pop_correct_index(self):
118+ """Test the pop method when working with a correct index."""
119+ value = [1, 2, 3, 4, 5]
120+ self.record["subfield_uuid"] = value
121+ # test with negative index
122+ poped_value = self.record["subfield_uuid"].pop(-2)
123+ self.assertEqual(value[-2], poped_value)
124+ # test with positive index
125+ poped_value = self.record["subfield_uuid"].pop(1)
126+ self.assertEqual(value[1], poped_value)
127+
128+ def test_mergeable_list_pop_wrong_index(self):
129+ """Test pop when index is out or range."""
130+ value = [1, 2, 3, 4, 5]
131+ self.record["subfield_uuid"] = value
132+ self.assertRaises(IndexError, self.record["subfield_uuid"].pop,
133+ len(value)*2)
134+
135+ def test_mergeable_list_pop_last(self):
136+ """Test that exception is raised when poping last item"""
137+ self.record["subfield_uuid"] = [1]
138+ self.assertRaises(ValueError, self.record["subfield_uuid"].pop, 0)
139+
140 def test_tuple(self):
141 """Test assigning tuples to a key results in mergeable lists."""
142 rec = Record({'record_type': 'http://fnord.org/smorgasbord'})

Subscribers

People subscribed via source and target branches