Merge lp:~jml/gtg/more-tests into lp:~gtg/gtg/old-trunk

[Jonathan Lange]

[Jonathan Lange (jml)]

The main purpose of this branch is to add tests for the Tag class.

The tests that I've added were designed to verify the current behaviour of the class. All of the tests pass running against the version of Tag that's in trunk. Two of the tests check behaviour that looks to me to be accidental, but I figured I'd add them and let someone who knows more than me make an informed decision.

Anyway, once the tests were written I took the opportunity to:
  a) Add docstrings for Tag
  b) Clean up whitespace
  c) Fix PEP 8 violations (remove spaces before colons, add spaces after commas, drop haskey usage)
  d) Changed the internal attributes to have underscore prefixes.
  e) Removed the "defensive" thing in get_all_attributes, since if ever 'name' isn't an attribute, it's actually a serious error.
  f) Changed set_attribute so that 'name' is slightly less special cased.

I think all of these changes are good ones, but tastes may vary.

jml

=== modified file 'GTG/core/tagstore.py'
--- GTG/core/tagstore.py	2009-03-31 09:29:10 +0000
+++ GTG/core/tagstore.py	2009-07-17 11:39:38 +0000
@@ -149,45 +149,71 @@
 #########################################################################
 ######################### Tag ###########################################
 
-#A tag is defined by its name (in most cases, it will be "@something") and it can have multiple attributes
-class Tag :
-
-    def __init__(self,name,save_cllbk=None) :
-        self.attributes = {}
-        self.name = name
-        self.set_attribute("name",self.name)
-        self.save = save_cllbk
-        
-    def get_name(self) :
+class Tag:
+    """A short name that can be applied to Tasks.
+
+    I mean, surely you must know what a tag is by now. Think Gmail,
+    del.icio.us, Flickr et al.
+
+    A tag is defined by its name, which in most cases is '@something'. A tag
+    can also have multiple arbitrary attributes. The only attribute enforced
+    for tags is 'name', which always matches `Tag.get_name()`.
+    """
+
+    def __init__(self, name, save_cllbk=None):
+        """Construct a tag.
+
+        :param name: The name of the tag. Should be a string, generally a
+            short one.
+        :param save_cllbk: A nullary callable, called whenever an attribute
+            is set.
+        """
+        self._name = str(name)
+        self._attributes = {'name': self._name}
+        self._save = save_cllbk
+
+    def get_name(self):
+        """Return the name of the tag."""
         return self.get_attribute("name")
-        
-    def set_attribute(self,att_name,att_value) :
-        #warning : only the constructor can set the "name"  
-        if att_name != "name" :
-            #Attributes should all be strings
-            val = unicode(str(att_value),"UTF-8")
-            self.attributes[att_name] = val
-            self.save()
-        elif self.name == att_value :
-            self.attributes[att_name] = str(att_value)
-        
-    def get_attribute(self,att_name) :
-        if self.attributes.has_key(att_name) :
-            return self.attributes[att_name]
-        else :
-            return None
-            
-    #if butname argument is set, the "name" attributes is removed
-    #from the list
-    def get_all_attributes(self,butname=False) :
-        l = self.attributes.keys()
-        if butname :
-            #Normally this condition is not necessary
-            #Defensiveness...
-            if "name" in l :
-                l.remove("name")
-        return l
-        
+
+    def set_attribute(self, att_name, att_value):
+        """Set an arbitrary attribute.
+
+        This will call the 'save_cllbk' callback passed to the constructor.
+
+        :param att_name: The name of the attribute.
+        :param att_value: The value of the attribute. Will be converted to a
+            string.
+        """
+        if att_name == "name":
+            # Warning : only the constructor can set the "name".
+            #
+            # XXX: This should actually raise an exception, or warn, or
+            # something. The Zen of Python says "Errors should never pass
+            # silently." -- jml, 2009-07-17
+            return
+        # Attributes should all be strings.
+        val = unicode(str(att_value), "UTF-8")
+        self._attributes[att_name] = val
+        self._save()
+
+    def get_attribute(self, att_name):
+        """Get the attribute 'att_name'.
+
+        Returns None if there is no attribute matching 'att_name'.
+        """
+        return self._attributes.get(att_name, None)
+
+    def get_all_attributes(self, butname=False):
+        """Return a list of all attribute names.
+
+        :param butname: If True, exclude 'name' from the list of attribute
+            names.
+        """
+        attributes = self._attributes.keys()
+        if butname:
+            attributes.remove('name')
+        return attributes
+
     def __str__(self):
-        return "Tag: %s" %self.get_name()
-
+        return "Tag: %s" % self.get_name()
 
=== modified file 'GTG/tests/__init__.py'
--- GTG/tests/__init__.py	2009-07-14 12:09:55 +0000
+++ GTG/tests/__init__.py	2009-07-17 08:35:00 +0000
@@ -21,10 +21,14 @@
 
 import unittest
 
-from GTG.tests import test_backends
+from GTG.tests import (
+    test_backends,
+    test_tagstore,
+    )
 
 
 def test_suite():
     return unittest.TestSuite([
         test_backends.test_suite(),
+        test_tagstore.test_suite(),
         ])
 
=== added file 'GTG/tests/test_tagstore.py'
--- GTG/tests/test_tagstore.py	1970-01-01 00:00:00 +0000
+++ GTG/tests/test_tagstore.py	2009-07-17 10:24:08 +0000
@@ -0,0 +1,122 @@
+# -*- coding: utf-8 -*-
+# -----------------------------------------------------------------------------
+# Gettings Things Gnome! - a personnal organizer for the GNOME desktop
+# Copyright (c) 2008-2009 - Lionel Dricot & Bertrand Rousseau
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free Software
+# Foundation, either version 3 of the License, or (at your option) any later
+# version.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+# details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program.  If not, see <http://www.gnu.org/licenses/>.
+# -----------------------------------------------------------------------------
+
+"""Tests for the tagstore."""
+
+import unittest
+
+from GTG.core.tagstore import Tag
+
+
+class TestTag(unittest.TestCase):
+    """Tests for `Tag`."""
+
+    def test_name(self):
+        # The first argument to the Tag constructor is the name, which you can
+        # get with get_name().
+        tag = Tag('foo')
+        self.assertEqual('foo', tag.get_name())
+
+    def test_name_is_attribute(self):
+        # The name of the tag is also stored as an attribute.
+        tag = Tag('foo')
+        self.assertEqual('foo', tag.get_attribute('name'))
+
+    def test_missing_attribute_returns_none(self):
+        # If get_attribute is called for an attribute that doesn't exist, it
+        # returns None.
+        tag = Tag('whatever')
+        result = tag.get_attribute('no-such-attribute')
+        self.assertEqual(None, result)
+
+    def test_set_attribute_calls_save(self):
+        # Calling set_attribute calls the 'save_cllbk'.
+        save_calls = []
+        tag = Tag('whatever', lambda: save_calls.append(None))
+        tag.set_attribute('new-attribute', 'value')
+        self.assertEqual(1, len(save_calls))
+
+    def test_set_then_get_attribute(self):
+        # Attributes set with set_attribute can be retrieved with
+        # get_attribute.
+        tag = Tag('whatever', lambda: None)
+        tag.set_attribute('new-attribute', 'value')
+        result = tag.get_attribute('new-attribute')
+        self.assertEqual('value', result)
+
+    def test_set_non_str_attribute_casts_to_string(self):
+        # If the value of the attribute passed to set_attribute is not a
+        # string, it's cast to a string.
+        tag = Tag('whatever', lambda: None)
+        tag.set_attribute('new-attribute', 42)
+        result = tag.get_attribute('new-attribute')
+        self.assertEqual('42', result)
+
+    def test_get_all_attributes_initial(self):
+        # Initially, a Tag has only the name attribute.
+        tag = Tag('foo')
+        self.assertEqual(['name'], tag.get_all_attributes())
+
+    def test_get_all_attributes_after_setting(self):
+        # After attributes are set, get_all_attributes includes those
+        # attributes. The order is not guaranteed.
+        tag = Tag('foo', lambda: None)
+        tag.set_attribute('bar', 'baz')
+        self.assertEqual(set(['name', 'bar']), set(tag.get_all_attributes()))
+
+    def test_get_all_but_name(self):
+        # If 'butname' is True, then exclude the 'name' attribute.
+        tag = Tag('foo', lambda: None)
+        self.assertEqual([], tag.get_all_attributes(butname=True))
+        tag.set_attribute('bar', 'baz')
+        self.assertEqual(['bar'], tag.get_all_attributes(butname=True))
+
+    def test_str(self):
+        # str(tag) is 'Tag: <name>'
+        tag = Tag('foo')
+        self.assertEqual('Tag: foo', str(tag))
+
+    def test_set_name_attribute_does_nothing(self):
+        # The 'name' attribute is set by the constructor. After it is set, it
+        # cannot be changed with further calls to set_attribute.
+        tag = Tag('old', lambda: None)
+        tag.set_attribute('name', 'new')
+        self.assertEqual('old', tag.get_name())
+        self.assertEqual('old', tag.get_attribute('name'))
+
+    # XXX: The following tests check the current behaviour of the Tag class,
+    # but I'm not sure if they're correct behaviour. -- jml, 2009-07-17
+
+    def test_save_not_called_on_construction(self):
+        # The save callback isn't called by the constructor, despite the fact
+        # that it sets the name attribute.
+        save_calls = []
+        Tag('old', lambda: save_calls.append(None))
+        self.assertEqual(0, len(save_calls))
+
+    def test_set_name_doesnt_call_save(self):
+        # Setting the name attribute doesn't call save.
+        save_calls = []
+        tag = Tag('old', lambda: save_calls.append(None))
+        tag.set_attribute('name', 'new')
+        self.assertEqual(0, len(save_calls))
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)