Cleaned up Env.__setattr__() and Env.__setitem__() a bit updated their unit tests

5 files changed, 120 insertions, 76 deletions

diff --git a/ipalib/config.py b/ipalib/config.py
index 8ff45dd9e..7f12b4256 100644
--- a/ipalib/config.py
+++ b/ipalib/config.py
@@ -30,7 +30,8 @@ from types import NoneType
 import os
 from os import path
 import sys
-from constants import CONFIG_SECTION, TYPE_ERROR, OVERRIDE_ERROR
+from constants import CONFIG_SECTION
+from constants import TYPE_ERROR, OVERRIDE_ERROR, LOCK_ERROR
 
 
 
@@ -51,6 +52,42 @@ class Env(object):
         self.home = path.abspath(os.environ['HOME'])
         self.dot_ipa = path.join(self.home, '.ipa')
 
+    def __setattr__(self, name, value):
+        """
+        Set the attribute named ``name`` to ``value``.
+
+        This just calls `Env.__setitem__()`.
+        """
+        self[name] = value
+
+    def __setitem__(self, key, value):
+        """
+        Set ``key`` to ``value``.
+        """
+        # FIXME: the key should be checked with check_name()
+        if self.__locked:
+            raise AttributeError(
+                LOCK_ERROR % (self.__class__.__name__, key, value)
+            )
+        if key in self.__d:
+            raise AttributeError(OVERRIDE_ERROR %
+                (self.__class__.__name__, key, self.__d[key], value)
+            )
+        if isinstance(value, basestring):
+            value = str(value.strip())
+            m = {
+                'True': True,
+                'False': False,
+                'None': None,
+            }
+            if value in m:
+                value = m[value]
+            elif value.isdigit():
+                value = int(value)
+        assert type(value) in (str, int, bool, NoneType)
+        object.__setattr__(self, key, value)
+        self.__d[key] = value
+
     def __doing(self, name):
         if name in self.__done:
             raise StandardError(
@@ -175,11 +212,7 @@ class Env(object):
     def __islocked__(self):
         return self.__locked
 
-    def __setattr__(self, name, value):
-        """
-        Set the attribute named ``name`` to ``value``.
-        """
-        self[name] = value
+
 
     def __delattr__(self, name):
         """
@@ -195,33 +228,7 @@ class Env(object):
         """
         return self.__d[key]
 
-    def __setitem__(self, key, value):
-        """
-        Set ``key`` to ``value``.
-        """
-        # FIXME: the key should be checked with check_name()
-        if self.__locked:
-            raise AttributeError('locked: cannot set %s.%s to %r' %
-                (self.__class__.__name__, key, value)
-            )
-        if key in self.__d or hasattr(self, key):
-            raise AttributeError('cannot overwrite %s.%s with %r' %
-                (self.__class__.__name__, key, value)
-            )
-        if isinstance(value, basestring):
-            value = str(value.strip())
-            m = {
-                'True': True,
-                'False': False,
-                'None': None,
-            }
-            if value in m:
-                value = m[value]
-            elif value.isdigit():
-                value = int(value)
-        assert type(value) in (str, int, bool, type(NoneType))
-        object.__setattr__(self, key, value)
-        self.__d[key] = value
+
 
     def __contains__(self, key):
         """
diff --git a/ipalib/constants.py b/ipalib/constants.py
index ef7de44c3..c74808d6d 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -32,7 +32,12 @@ TYPE_ERROR = '%s: need a %r; got %r (which is a %r)'
 CALLABLE_ERROR = '%s: need a callable; got %r (which is a %r)'
 
 # Standard format for StandardError message when overriding an attribute:
-OVERRIDE_ERROR = 'cannot override %s value %r with %r'
+OVERRIDE_ERROR = 'cannot override %s.%s value %r with %r'
+
+# Standard format for AttributeError message when a read-only attribute is
+# already locked:
+LOCK_ERROR = 'locked: cannot set %s.%s to %r'
+DEL_ERROR = 'locked: cannot set %s.%s to %r'
 
 # Used for a tab (or indentation level) when formatting for CLI:
 CLI_TAB = '  '  # Two spaces
diff --git a/ipalib/request.py b/ipalib/request.py
index 545ebc540..ea028239c 100644
--- a/ipalib/request.py
+++ b/ipalib/request.py
@@ -34,8 +34,8 @@ context = threading.local()
 
 def set_languages(*languages):
     if hasattr(context, 'languages'):
-        raise StandardError(
-            OVERRIDE_ERROR % ('context.languages', context.languages, languages)
+        raise StandardError(OVERRIDE_ERROR %
+            ('context', 'languages', context.languages, languages)
         )
     if len(languages) == 0:
         languages = locale.getdefaultlocale()[:1]
diff --git a/tests/test_ipalib/test_config.py b/tests/test_ipalib/test_config.py
index f92445f8a..c1a5faaa0 100644
--- a/tests/test_ipalib/test_config.py
+++ b/tests/test_ipalib/test_config.py
@@ -28,10 +28,33 @@ import sys
 from tests.util import raises, setitem, delitem, ClassChecker
 from tests.util import getitem, setitem, delitem
 from tests.util import TempDir, TempHome
+from ipalib.constants import TYPE_ERROR, OVERRIDE_ERROR, LOCK_ERROR
 from ipalib import config, constants
 
 
 
+# Valid environment variables in (key, raw, value) tuples:
+#    key: the name of the environment variable
+#    raw: the value being set (possibly a string repr)
+#    value: the expected value after the lightweight conversion
+good_vars = (
+    ('a_string', 'Hello world!', 'Hello world!'),
+    ('trailing_whitespace', ' value  ', 'value'),
+    ('an_int', 42, 42),
+    ('int_repr', ' 42 ', 42),
+    ('true', True, True),
+    ('true_repr', ' True ', True),
+    ('false', False, False),
+    ('false_repr', ' False ', False),
+    ('none', None, None),
+    ('none_repr', ' None ', None),
+
+    # These verify that the implied conversion is case-sensitive:
+    ('not_true', ' true ', 'true'),
+    ('not_false', ' false ', 'false'),
+    ('not_none', ' none ', 'none'),
+)
+
 
 
 # Random base64-encoded data to simulate a misbehaving config file.
@@ -132,6 +155,54 @@ class test_Env(ClassChecker):
         assert o.home == home.path
         assert o.dot_ipa == home.join('.ipa')
 
+    def test_setattr(self):
+        """
+        Test the `ipalib.config.Env.__setattr__` method.
+        """
+        o = self.cls()
+        for (name, raw, value) in good_vars:
+            # Test setting the value:
+            setattr(o, name, raw)
+            result = getattr(o, name)
+            assert type(result) is type(value)
+            assert result == value
+            assert result is o[name]
+
+            # Test that value cannot be overridden once set:
+            e = raises(AttributeError, setattr, o, name, raw)
+            assert str(e) == OVERRIDE_ERROR % ('Env', name, value, raw)
+
+        # Test that values cannot be set once locked:
+        o = self.cls()
+        o.__lock__()
+        for (name, raw, value) in good_vars:
+            e = raises(AttributeError, setattr, o, name, raw)
+            assert str(e) == LOCK_ERROR % ('Env', name, raw)
+
+    def test_setitem(self):
+        """
+        Test the `ipalib.config.Env.__setitem__` method.
+        """
+        o = self.cls()
+        for (key, raw, value) in good_vars:
+            # Test setting the value:
+            o[key] = raw
+            result = o[key]
+            assert type(result) is type(value)
+            assert result == value
+            assert result is getattr(o, key)
+
+            # Test that value cannot be overridden once set:
+            e = raises(AttributeError, o.__setitem__, key, raw)
+            assert str(e) == OVERRIDE_ERROR % ('Env', key, value, raw)
+
+        # Test that values cannot be set once locked:
+        o = self.cls()
+        o.__lock__()
+        for (key, raw, value) in good_vars:
+            e = raises(AttributeError, o.__setitem__, key, raw)
+            assert str(e) == LOCK_ERROR % ('Env', key, raw)
+
     def bootstrap(self, **overrides):
         (o, home) = self.new()
         assert o._isdone('_bootstrap') is False
@@ -387,46 +458,7 @@ class test_Env(ClassChecker):
             e = raises(KeyError, getitem, o, name)
             assert str(e) == repr(name)
 
-    def test_setattr(self):
-        """
-        Test the `ipalib.config.Env.__setattr__` method.
 
-        Also tests the `ipalib.config.Env.__setitem__` method.
-        """
-        items = [
-            ('one', 1),
-            ('two', 2),
-            ('three', 3),
-            ('four', 4),
-        ]
-        for setvar in (setattr, setitem):
-            o = self.cls()
-            for (i, (name, value)) in enumerate(items):
-                setvar(o, name, value)
-                assert getattr(o, name) == i + 1
-                assert o[name] == i + 1
-                if callable(value):
-                    assert name not in dir(o)
-                else:
-                    assert name in dir(o)
-                e = raises(AttributeError, setvar, o, name, 42)
-                assert str(e) == 'cannot overwrite Env.%s with 42' % name
-            o = self.cls()
-            o.__lock__()
-            for (name, value) in items:
-                e = raises(AttributeError, setvar, o, name, value)
-                assert str(e) == \
-                    'locked: cannot set Env.%s to %r' % (name, value)
-            o = self.cls()
-            setvar(o, 'yes', ' True ')
-            assert o.yes is True
-            setvar(o, 'no', ' False ')
-            assert o.no is False
-            setvar(o, 'msg', u' Hello, world! ')
-            assert o.msg == 'Hello, world!'
-            assert type(o.msg) is str
-            setvar(o, 'num', ' 42 ')
-            assert o.num == 42
 
     def test_delattr(self):
         """
diff --git a/tests/test_ipalib/test_request.py b/tests/test_ipalib/test_request.py
index 1b9c9e3d2..5efc0abdb 100644
--- a/tests/test_ipalib/test_request.py
+++ b/tests/test_ipalib/test_request.py
@@ -40,7 +40,7 @@ def test_set_languages():
     assert not hasattr(c, 'languages')
     c.languages = None
     e = raises(StandardError, f, *langs)
-    assert str(e) == OVERRIDE_ERROR % ('context.languages', None, langs)
+    assert str(e) == OVERRIDE_ERROR % ('context', 'languages', None, langs)
     del c.languages
 
     # Test setting the languages: