Merge lp:~thisfred/desktopcouch/remove-recordtype-from-dict into lp:desktopcouch

[Eric Casteleijn]

Fixes #674487 where the record_type ended up in the dictionary of the record, where almost all client code had to filter it out again.
Fixes #669133 because the record_type version was never saved to CouchDB.
Fixes #575772 where the couchdb attachments API was defined on the wrong class.
Fixes #675787 where MergeableList.pop() could not be called without an index.

Fixed too much on this branch because I got a little frustrated with the number of bugs I ran into. If it's impossible to review, please tell me and I'll try and split all the fixes out into their own branches...

[Eric Casteleijn (thisfred)]

Fixes #674487 where the record_type ended up in the dictionary of the record, where almost all client code had to filter it out again.
Fixes #669133 because the record_type version was never saved to CouchDB.
Fixes #575772 where the couchdb attachments API was defined on the wrong class.
Fixes #675787 where MergeableList.pop() could not be called without an index.

[Eric Casteleijn (thisfred)]

Note that the frustration is mostly aimed at myself, since this shows that I suck at reviewing and writing code, since I did at least one of those for all of the code that was buggy.

[Nicola Larosa (teknico)]

I like this branch. A couple comments:

- since you don't need current_value anymore here:

- for index, current_value in enumerate(self):
+ for index, _ in enumerate(self):

you might as well write:

     for index in range(len(self)):

Also, what's with the double blank lines before or after functions and methods? You only need double between classes. Don't get overboard with that automatic pep8 thing. ;-)

review: Approve

[Eric Casteleijn (thisfred)]

> - since you don't need current_value anymore here:
>
> - for index, current_value in enumerate(self):
> + for index, _ in enumerate(self):
>
> you might as well write:
>
> for index in range(len(self)):

Yeah that's better

> Also, what's with the double blank lines before or after functions and
> methods? You only need double between classes. Don't get overboard with that
> automatic pep8 thing. ;-)

Actually I like having two lines between top level definitions no matter what they are (so functions or Classes, *not* methods.) It makes me have to think less, and improves readability. Also a top level function and a class definition are rather similar things IMO.

[lp:~thisfred/desktopcouch/remove-recordtype-from-dict]

218. By Eric Casteleijn on 2010-11-15

microbeautification

[Vincenzo Di Somma (vds)]

review: Approve

=== modified file 'desktopcouch/contacts/tests/test_record.py'
--- desktopcouch/contacts/tests/test_record.py	2010-10-31 23:20:14 +0000
+++ desktopcouch/contacts/tests/test_record.py	2010-11-15 23:22:02 +0000
@@ -46,6 +46,5 @@
         first_name = 'manuel'
         contact.first_name = first_name
         self.assertEqual(first_name, contact.first_name)
-        self.assertEqual(set(['record_type', 'first_name']),
-                         set(contact.keys()))
-        
+        self.assertEqual(set(['first_name']), set(contact.keys()))
+
 
=== modified file 'desktopcouch/records/doc/an_example_application.txt'
--- desktopcouch/records/doc/an_example_application.txt	2010-11-03 22:41:43 +0000
+++ desktopcouch/records/doc/an_example_application.txt	2010-11-15 23:22:02 +0000
@@ -116,7 +116,7 @@
 kinds of dynamic coding easier:
 
     >>> sorted([key for key in my_recipe])
-    ['ingredients', 'name', 'record_type']
+    ['ingredients', 'name']
 
 There are more specific types of RecordField available if you want to
 encourage that the value of one of the fields is of a certain
 
=== modified file 'desktopcouch/records/record.py'
--- desktopcouch/records/record.py	2010-11-03 22:41:43 +0000
+++ desktopcouch/records/record.py	2010-11-15 23:22:02 +0000
@@ -27,9 +27,13 @@
 
 RX = re.compile('[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}')
 
+
 def is_internal(key):
     """Test whether this is an internal key."""
-    return key.startswith(('_', 'application_annotations'))
+    return (
+        key == 'record_type' or
+        key.startswith(('_', 'application_annotations')))
+
 
 def is_uuid_dict(dictionary):
     """Test whether the passed object is a dictionary like object with
@@ -38,12 +42,14 @@
     return dictionary and not [
         key for key in dictionary if (not RX.match(key) and key != '_order')]
 
+
 def record_factory(data):
     """Create a RecordDict or RecordList from passed data."""
     if isinstance(data, dict):
         return _build_from_dict(data)
     return data
 
+
 def _build_from_dict(data):
     """Create a RecordDict from passed data."""
     result = RecordDict({})
@@ -54,6 +60,7 @@
         result[key] = record_factory(value)
     return result
 
+
 def validate(data):
     """Validate underlying data for Record objects."""
     if isinstance(data, dict):
@@ -73,7 +80,7 @@
     def __init__(self, field_name):
         self.field_name = field_name
 
-    def __get__(self, obj, type=None):
+    def __get__(self, obj, owner=None):
         return obj.get(self.field_name)
 
     def __set__(self, obj, value):
@@ -109,6 +116,7 @@
     """Exception when attempting to set a key we don't allow."""
     pass
 
+
 class NoRecordTypeSpecified(Exception):
     """Exception when creating a record without specifying record type"""
     def __str__(self):
@@ -118,6 +126,7 @@
 class Attachment(object):
     """This represents the value of the attachment.  The name is
     stored as the key that points to this name."""
+
     def __init__(self, content=None, content_type=None):
         super(Attachment, self).__init__()
         if content is not None:
@@ -132,7 +141,8 @@
         self.content_type = content_type
         self.needs_synch = content is not None
 
-    def get_content_and_type(self):
+    def get_content_and_type(self):     # WTF? pylint: disable=E0202
+        """Get content and type of the attachment."""
         assert self.content is not None
         if hasattr(self.content, "read"):
             if hasattr(self.content, "seek"):
@@ -165,7 +175,7 @@
         if isinstance(item, dict):
             item = record_factory(item)
         if hasattr(item, '_data'):
-            item = item._data
+            item = item._data           # pylint: disable=W0212
         self._data[key] = item
 
     def __delitem__(self, key):
@@ -174,44 +184,11 @@
     def __len__(self):
         return len(self._data)
 
-    def attach_by_reference(self, filename, getter_function):
-        """This should only be called by the server code to refer to a BLOB
-        that is in the database, so that we do not have to download it until
-        the user wants it."""
-        a = Attachment(None, None)
-        a.get_content_and_type = getter_function
-        self._attachments[filename] = a
-
-    def attach(self, content, filename, content_type):
-        """Attach a file-like or string-like object, with a particular name and
-        content type, to a document to be stored in the database.  One can not
-        clobber names that already exist."""
-        if filename in self._attachments:
-            raise KeyError("%r already exists" % (filename,))
-        self._attachments[filename] = Attachment(content, content_type)
-
-    def detach(self, filename):
-        """Remove a BLOB attached to a document."""
-        try:
-            self._attachments.pop(filename)
-            self._detached.add(filename)
-        except KeyError:
-            raise KeyError("%r is not attached to this document" % (filename,))
-
-    def list_attachments(self):
-        return self._attachments.keys()
-
-    def attachment_data(self, filename):
-        """Retreive the attached data, the BLOB and content_type."""
-        try:
-            a = self._attachments[filename]
-        except KeyError:
-            raise KeyError("%r is not attached to this document" % (filename,))
-        return a.get_content_and_type()
 
 class RecordDict(RecordData):
     """An object that represents an desktop CouchDB record fragment,
     but behaves like a dictionary.
+
     """
 
     def __setitem__(self, key, item):
@@ -229,48 +206,54 @@
 
     def __cmp__(self, value):
         if isinstance(value, RecordDict):
-            return cmp(self._data, value._data)
+            return cmp(self._data, value._data)  # pylint: disable=W0212
         return -1
 
     def get(self, key, default=None):
-        """Override get method."""
+        """D.get(k[,d]) -> D[k] if k in D, else d.  d defaults to None."""
         if not key in self._data:
             return default
         return self[key]
 
     def update(self, data):
-        """Set items from a dict"""
+        """D.update(E, **F) -> None.  Update D from dict/iterable E and F.
+
+        If E has a .keys() method, does:     for k in E: D[k] = E[k]
+        If E lacks .keys() method, does:     for (k, v) in E: D[k] = v
+        In either case, this is followed by: for k in F: D[k] = F[k]
+
+        """
         for key, val in data.items():
             self[key] = val
 
     def items(self):
-        """Return all items."""
+        """D.items() -> list of D's (key, value) pairs, as 2-tuples."""
         return [(key, self[key]) for key in self]
 
     def keys(self):
-        """Return all keys."""
+        """D.keys() -> list of D's keys."""
         return self._data.keys()
 
     def setdefault(self, key, default):
-        """remap setdefault"""
+        """D.setdefault(k[,d]) -> D.get(k,d), also set D[k]=d if k not in D."""
         if not key in self:
             self[key] = default
         return self[key]
 
     def has_key(self, key):
+        """D.has_key(k) -> True if D has a key k, else False."""
         return key in self
 
 
-
 class MergeableList(RecordData):
     """An object that represents a list of complex values."""
 
     @classmethod
-    def from_list(klass, data):
+    def from_list(cls, data):
         """Create a RecordList from passed data."""
         if not data:
             raise ValueError("Can't set empty list values.'")
-        result = klass({})
+        result = cls({})
         for value in data:
             result.append(record_factory(value))
         return result
@@ -309,7 +292,7 @@
 
     def __contains__(self, value):
         if hasattr(value, '_data'):
-            value = value._data
+            value = value._data         # pylint: disable=W0212
         return value in self._data.values()
 
     def __cmp__(self, value):
@@ -321,9 +304,9 @@
             and hasattr(value, "__getitem__")
             and hasattr(value, "__len__")):
             # we should have the same value in the same order
-            length  = len(value)
+            length = len(value)
             if len(self) == length:
-                for index, current_value in enumerate(self):
+                for index in range(len(self)):
                     cmp_value = cmp(self[index], value[index])
                     if cmp_value != 0:
                         return cmp_value
@@ -345,20 +328,25 @@
         """Return uuid key for index."""
         return self._get_ordered_keys()[index]
 
-    def get_value_for_uuid(self, uuid):
+    def get_value_for_uuid(self, lookup_uuid):
         """Allow API consumers that do know about the UUIDs to get
         values directly.
         """
-        return super(MergeableList, self).__getitem__(uuid)
+        return super(MergeableList, self).__getitem__(lookup_uuid)
 
     def append(self, value):
-        """Append a value to end of MergeableList."""
+        """L.append(object) -- append object to end."""
         new_uuid = str(uuid.uuid4())
         self._data.setdefault('_order', sorted(self._data.keys())).append(
             new_uuid)
         super(MergeableList, self).__setitem__(new_uuid, value)
 
     def remove(self, value):
+        """L.remove(value) -- remove first occurrence of value.
+
+        Raises ValueError if the value is not present.
+
+        """
         if len(self) == 1:
             raise ValueError("MergeableList cannot be empty.")
         index = 0
@@ -371,15 +359,26 @@
             index += 1
         raise ValueError("list.remove(x): x not in list")
 
-    def pop(self, index):
+    def pop(self, index=None):
+        """L.pop([index]) -> item -- remove and return item at
+        index. (default last.)
+
+        Raises IndexError if list is empty or index is out of range.
+        """
         if len(self) == 1:
             raise ValueError("MergeableList cannot be empty.")
+        if index is None:
+            index = -1
         value = self[index]
         del self[index]
         return value
 
     def index(self, key):
-        """Get value by index."""
+        """L.index(value, [start, [stop]]) -> integer -- return first
+        index of value.
+
+        Raises ValueError if the value is not present.
+        """
         return self.__getitem__(key)
 
 
@@ -397,6 +396,8 @@
                 raise NoRecordTypeSpecified
         super(Record, self).__init__(data)
         self._data['record_type'] = record_type
+        if record_type_version is not None:
+            self._data['_record_type_version'] = record_type_version
 
         if record_id is not None:
             # Explicit setting, so try to use it.
@@ -407,8 +408,6 @@
                 # _id already set.  Verify that the explicit value agrees.
                 if self._data['_id'] != record_id:
                     raise ValueError("record_id doesn't match value in data.")
-        if record_type_version:
-            self._record_type_version = record_type_version
 
     def __getitem__(self, key):
         if is_internal(key):
@@ -481,4 +480,40 @@
     @property
     def record_type_version(self):
         """Get the record type version if it's there."""
-        return getattr(self, '_record_type_version', None)
+        return self._data.get('_record_type_version', None)
+
+    def attach_by_reference(self, filename, getter_function):
+        """This should only be called by the server code to refer to a BLOB
+        that is in the database, so that we do not have to download it until
+        the user wants it."""
+        a = Attachment(None, None)
+        a.get_content_and_type = getter_function
+        self._attachments[filename] = a
+
+    def attach(self, content, filename, content_type):
+        """Attach a file-like or string-like object, with a particular name and
+        content type, to a document to be stored in the database.  One can not
+        clobber names that already exist."""
+        if filename in self._attachments:
+            raise KeyError("%r already exists" % (filename,))
+        self._attachments[filename] = Attachment(content, content_type)
+
+    def detach(self, filename):
+        """Remove a BLOB attached to a document."""
+        try:
+            self._attachments.pop(filename)
+            self._detached.add(filename)
+        except KeyError:
+            raise KeyError("%r is not attached to this document" % (filename,))
+
+    def list_attachments(self):
+        """List the record's attachments."""
+        return self._attachments.keys()
+
+    def attachment_data(self, filename):
+        """Retreive the attached data, the BLOB and content_type."""
+        try:
+            a = self._attachments[filename]
+        except KeyError:
+            raise KeyError("%r is not attached to this document" % (filename,))
+        return a.get_content_and_type()
 
=== modified file 'desktopcouch/records/tests/test_record.py'
--- desktopcouch/records/tests/test_record.py	2010-11-12 14:34:21 +0000
+++ desktopcouch/records/tests/test_record.py	2010-11-15 23:22:02 +0000
@@ -98,7 +98,7 @@
     def test_iter(self):
         """Tests it is possible to iterate over the record fields"""
         self.assertEquals(sorted(list(iter(self.record))),
-            ['a', 'b', 'record_type', 'subfield', 'subfield_uuid'])
+            ['a', 'b', 'subfield', 'subfield_uuid'])
 
     def test_setitem_internal(self):
         """Test it is not possible to set a private field on a record"""
@@ -125,7 +125,7 @@
     def test_keys(self):
         """Test getting the keys."""
         self.assertEqual(
-            ['a', 'b', 'record_type', 'subfield', 'subfield_uuid'],
+            ['a', 'b', 'subfield', 'subfield_uuid'],
             sorted(self.record.keys()))
         self.assertIn("a", self.record)
         self.assertNotIn('f', self.record)
@@ -248,10 +248,18 @@
     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.record["subfield_uuid"] = MergeableList.from_list(value)
         self.assertRaises(IndexError, self.record["subfield_uuid"].pop,
             len(value) * 2)
 
+    def test_mergeable_list_pop_no_index(self):
+        """Test pop without an index argument."""
+        value = [1, 2, 3, 4, 5]
+        self.record["subfield_uuid"] = MergeableList.from_list(value)
+        popped_value = self.record["subfield_uuid"].pop()
+        self.assertEqual(value[-1], popped_value)
+
+
     def test_mergeable_list_pop_last(self):
         """Test that exception is raised when poping last item"""
         self.record["subfield_uuid"] = MergeableList.from_list([1])
@@ -353,9 +361,13 @@
         """Test record type version support"""
         data = {"_id": "recordid"}
         record1 = Record(data, record_type="url")
+        self.assertIs(None, record1._data.get(  # pylint: disable=W0212
+            '_record_type_version'))
         self.assertIs(None, record1.record_type_version)
-        record2 = Record(data, record_type="url", record_type_version=1)
-        self.assertEqual(record2.record_type_version, 1)
+        record2 = Record(data, record_type="url", record_type_version='1.0')
+        self.assertEqual(record2._data[  # pylint: disable=W0212
+            '_record_type_version'], '1.0')
+        self.assertEqual(record2.record_type_version, '1.0')
 
 
 class TestRecordFactory(TestCase):
 
=== modified file 'desktopcouch/records/tests/test_server.py'
--- desktopcouch/records/tests/test_server.py	2010-11-10 21:17:11 +0000
+++ desktopcouch/records/tests/test_server.py	2010-11-15 23:22:02 +0000
@@ -385,7 +385,8 @@
         results = self.database.get_all_records()
 
         self.assertTrue(8 <= len(results))  # our 8, plus callers' data
-        self.assertIn("record_type", results[0])
+        self.assertNotIn("record_type", results[0])
+        self.assertTrue(hasattr(results[0], "record_type"))
 
         for result in results:  # index notation
             if result.record_type == good_record_type:
 
=== modified file 'utilities/lint.sh'
--- utilities/lint.sh	2010-11-12 14:34:21 +0000
+++ utilities/lint.sh	2010-11-15 23:22:02 +0000
@@ -37,7 +37,7 @@
 fi
 
 export PYTHONPATH="/usr/share/pycentral/pylint/site-packages:desktopcouch:$PYTHONPATH"
-pylint="`which pylint` -r n -i y --variable-rgx=[a-z_][a-z0-9_]{0,30} --attr-rgx=[a-z_][a-z0-9_]{1,30} --method-rgx=[a-z_][a-z0-9_]{2,} -d I0011,R0902,R0904,R0913,W0142"
+pylint="`which pylint` -r n -i y --variable-rgx=[a-z_][a-z0-9_]{0,30} --attr-rgx=[a-z_][a-z0-9_]{1,30} --method-rgx=[a-z_][a-z0-9_]{2,} -d I0011,R0902,R0903,R0904,R0913,W0142"
 
 pylint_notices=`$pylint $pyfiles`