Merge lp:~vila/bzr/bool-config-option into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This patch adds the ability to interpret a configuration as a boolean.

It can be viewed (or reviewed :) as:
- add a bool_from_string() function,
- use that function to define config.get_user_option_as_bool()
- use that function in ui.get_boolean
- use config.get_user_option_as_bool() where needed

Regarding the discussion about defining a new method when we add a new configuration variable,
my opinion is that it's not always worth it.

The --strict option for push and send can be defined anywhere, I prefer to handle the
default value in the commands themselves because:
- that's the only place they are used,
- that's the place that is targeted by the tests

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/bool-config-option into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This patch adds the ability to interpret a configuration as a boolean.
>
> It can be viewed (or reviewed :) as:
> - add a bool_from_string() function,
> - use that function to define config.get_user_option_as_bool()
> - use that function in ui.get_boolean
> - use config.get_user_option_as_bool() where needed
>
> Regarding the discussion about defining a new method when we add a new configuration variable,
> my opinion is that it's not always worth it.
>
> The --strict option for push and send can be defined anywhere, I prefer to handle the
> default value in the commands themselves because:
> - that's the only place they are used,
> - that's the place that is targeted by the tests
>

Though it also makes it hard to know the full list of config items that
one could possibly set without using "grep -rnI get_user_option bzrlib"

Anyway, this patch would probably be fine if lp:mad wasn't crazy.

  review resubmit

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpM5tQACgkQJdeBCYSNAAPYeACfbcQUsCFWHvP5zJlzbXOtYr4r
idgAn3xS0cIKF70naJYUDm/4tv87C1tD
=O5+R
-----END PGP SIGNATURE-----

review: Needs Resubmitting

[Ian Clatworthy (ian-clatworthy)]

Looks all ok to me. I'd like to suggest though that the rstrip('\n') call in ui/__init__.py omit the parameter and default to stripping all trailing whitespace. That might work better on Windows? (Either way, I think trailing whitespace is harmless.)

review: Approve

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-07-02 21:17:35 +0000
+++ bzrlib/builtins.py	2009-07-09 07:35:50 +0000
@@ -1096,24 +1096,15 @@
         (tree, br_from,
          _unused) = bzrdir.BzrDir.open_containing_tree_or_branch(directory)
         if strict is None:
-            strict = br_from.get_config().get_user_option('push_strict')
-            if strict is not None:
-                # FIXME: This should be better supported by config
-                # -- vila 20090611
-                bools = dict(yes=True, no=False, on=True, off=False,
-                             true=True, false=False)
-                try:
-                    strict = bools[strict.lower()]
-                except KeyError:
-                    strict = None
+            strict = br_from.get_config().get_user_option_as_bool('push_strict')
+        if strict is None: strict = True # default value
         # Get the tip's revision_id
         revision = _get_one_revision('push', revision)
         if revision is not None:
             revision_id = revision.in_history(br_from).rev_id
         else:
             revision_id = None
-        if (tree is not None and revision_id is None
-            and (strict is None or strict)): # Default to True:
+        if strict and tree is not None and revision_id is None:
             changes = tree.changes_from(tree.basis_tree())
             if changes.has_changed() or len(tree.get_parent_ids()) > 1:
                 raise errors.UncommittedChanges(

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2009-06-11 06:49:21 +0000
+++ bzrlib/config.py	2009-07-09 07:35:50 +0000
@@ -146,6 +146,9 @@
 class Config(object):
     """A configuration policy - what username, editor, gpg needs etc."""
 
+    def __init__(self):
+        super(Config, self).__init__()
+
     def get_editor(self):
         """Get the users pop up editor."""
         raise NotImplementedError
@@ -174,6 +177,15 @@
         """Get a generic option - no special process, no default."""
         return self._get_user_option(option_name)
 
+    def get_user_option_as_bool(self, option_name):
+        """Get a generic option as a boolean - no special process, no default.
+
+        :return None if the option doesn't exist or its value can't be
+            interpreted as a boolean. Returns True or False ortherwise.
+        """
+        s = self._get_user_option(option_name)
+        return ui.bool_from_string(s)
+
     def gpg_signing_command(self):
         """What program should be used to sign signatures?"""
         result = self._gpg_signing_command()
@@ -196,9 +208,6 @@
         """See log_format()."""
         return None
 
-    def __init__(self):
-        super(Config, self).__init__()
-
     def post_commit(self):
         """An ordered list of python functions to call.
 
@@ -299,6 +308,11 @@
 class IniBasedConfig(Config):
     """A configuration policy that draws from ini files."""
 
+    def __init__(self, get_filename):
+        super(IniBasedConfig, self).__init__()
+        self._get_filename = get_filename
+        self._parser = None
+
     def _get_parser(self, file=None):
         if self._parser is not None:
             return self._parser
@@ -381,11 +395,6 @@
         """See Config.log_format."""
         return self._get_user_option('log_format')
 
-    def __init__(self, get_filename):
-        super(IniBasedConfig, self).__init__()
-        self._get_filename = get_filename
-        self._parser = None
-
     def _post_commit(self):
         """See Config.post_commit."""
         return self._get_user_option('post_commit')

=== modified file 'bzrlib/send.py'
--- bzrlib/send.py	2009-07-01 15:17:33 +0000
+++ bzrlib/send.py	2009-07-09 07:35:50 +0000
@@ -108,17 +108,10 @@
                 base_revision_id = revision[0].as_revision_id(branch)
         if revision_id is None:
             if strict is None:
-                strict = branch.get_config().get_user_option('send_strict')
-                if strict is not None:
-                    # FIXME: This should be better supported by config
-                    # -- vila 20090626
-                    bools = dict(yes=True, no=False, on=True, off=False,
-                                 true=True, false=False)
-                    try:
-                        strict = bools[strict.lower()]
-                    except KeyError:
-                        strict = None
-            if tree is not None and (strict is None or strict):
+                strict = branch.get_config(
+                    ).get_user_option_as_bool('send_strict')
+            if strict is None: strict = True # default value
+            if strict and tree is not None:
                 changes = tree.changes_from(tree.basis_tree())
                 if changes.has_changed() or len(tree.get_parent_ids()) > 1:
                     raise errors.UncommittedChanges(

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2009-06-29 12:14:09 +0000
+++ bzrlib/tests/test_config.py	2009-07-09 07:35:50 +0000
@@ -367,6 +367,20 @@
         parser = my_config._get_parser(file=config_file)
         self.failUnless(my_config._get_parser() is parser)
 
+    def test_get_user_option_as_bool(self):
+        config_file = StringIO("""
+a_true_bool = true
+a_false_bool = 0
+an_invalid_bool = maybe
+a_list = hmm, who knows ? # This interpreted as a list !
+""".encode('utf-8'))
+        my_config = config.IniBasedConfig(None)
+        parser = my_config._get_parser(file=config_file)
+        get_option = my_config.get_user_option_as_bool
+        self.assertEqual(True, get_option('a_true_bool'))
+        self.assertEqual(False, get_option('a_false_bool'))
+        self.assertIs(None, get_option('an_invalid_bool'))
+        self.assertIs(None, get_option('not_defined_in_this_config'))
 
 class TestGetConfig(tests.TestCase):
 

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2009-06-30 05:34:47 +0000
+++ bzrlib/tests/test_ui.py	2009-07-09 07:35:50 +0000
@@ -174,21 +174,22 @@
         pb1.finished()
 
     def assert_get_bool_acceptance_of_user_input(self, factory):
-        factory.stdin = StringIO("y\nyes with garbage\n"
-                                 "yes\nn\nnot an answer\n"
-                                 "no\n"
-                                 "N\nY\n"
-                                 "foo\n"
-                                )
+        factory.stdin = StringIO("y\n" # True
+                                 "n\n" # False
+                                 "yes with garbage\nY\n" # True
+                                 "not an answer\nno\n" # False
+                                 "I'm sure!\nyes\n" # True
+                                 "NO\n" # False
+                                 "foo\n")
         factory.stdout = StringIO()
         factory.stderr = StringIO()
         # there is no output from the base factory
         self.assertEqual(True, factory.get_boolean(""))
-        self.assertEqual(True, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
-        self.assertEqual(False, factory.get_boolean(""))
-        self.assertEqual(True, factory.get_boolean(""))
+        self.assertEqual(False, factory.get_boolean(""))
+        self.assertEqual(True, factory.get_boolean(""))
+        self.assertEqual(False, factory.get_boolean(""))
+        self.assertEqual(True, factory.get_boolean(""))
+        self.assertEqual(False, factory.get_boolean(""))
         self.assertEqual("foo\n", factory.stdin.read())
         # stdin should be empty
         self.assertEqual('', factory.stdin.readline())
@@ -355,3 +356,58 @@
             r'[####|               ] a:b:c 1/2'
             , uif._progress_view._render_line())
 
+
+class TestBoolFromString(tests.TestCase):
+
+    def assertIsTrue(self, s, accepted_values=None):
+        res = _mod_ui.bool_from_string(s, accepted_values=accepted_values)
+        self.assertEquals(True, res)
+
+    def assertIsFalse(self, s, accepted_values=None):
+        res = _mod_ui.bool_from_string(s, accepted_values=accepted_values)
+        self.assertEquals(False, res)
+
+    def assertIsNone(self, s, accepted_values=None):
+        res = _mod_ui.bool_from_string(s, accepted_values=accepted_values)
+        self.assertIs(None, res)
+
+    def test_know_valid_values(self):
+        self.assertIsTrue('true')
+        self.assertIsFalse('false')
+        self.assertIsTrue('1')
+        self.assertIsFalse('0')
+        self.assertIsTrue('on')
+        self.assertIsFalse('off')
+        self.assertIsTrue('yes')
+        self.assertIsFalse('no')
+        self.assertIsTrue('y')
+        self.assertIsFalse('n')
+        # Also try some case variations
+        self.assertIsTrue('True')
+        self.assertIsFalse('False')
+        self.assertIsTrue('On')
+        self.assertIsFalse('Off')
+        self.assertIsTrue('ON')
+        self.assertIsFalse('OFF')
+        self.assertIsTrue('oN')
+        self.assertIsFalse('oFf')
+
+    def test_invalid_values(self):
+        self.assertIsNone(None)
+        self.assertIsNone('doubt')
+        self.assertIsNone('frue')
+        self.assertIsNone('talse')
+        self.assertIsNone('42')
+
+    def test_provided_values(self):
+        av = dict(y=True, n=False, yes=True, no=False)
+        self.assertIsTrue('y', av)
+        self.assertIsTrue('Y', av)
+        self.assertIsTrue('Yes', av)
+        self.assertIsFalse('n', av)
+        self.assertIsFalse('N', av)
+        self.assertIsFalse('No', av)
+        self.assertIsNone('1', av)
+        self.assertIsNone('0', av)
+        self.assertIsNone('on', av)
+        self.assertIsNone('off', av)

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-06-30 05:34:47 +0000
+++ bzrlib/ui/__init__.py	2009-07-09 07:35:50 +0000
@@ -43,6 +43,42 @@
 """)
 
 
+_valid_boolean_strings = dict(yes=True, no=False,
+                              y=True, n=False,
+                              on=True, off=False,
+                              true=True, false=False)
+_valid_boolean_strings['1'] = True
+_valid_boolean_strings['0'] = False
+
+
+def bool_from_string(s, accepted_values=None):
+    """Returns a boolean if the string can be interpreted as such.
+
+    Interpret case insensitive strings as booleans. The default values
+    includes: 'yes', 'no, 'y', 'n', 'true', 'false', '0', '1', 'on',
+    'off'. Alternative values can be provided with the 'accepted_values'
+    parameter.
+
+    :param s: A string that should be interpreted as a boolean. It should be of
+        type string or unicode.
+
+    :param accepted_values: An optional dict with accepted strings as keys and
+        True/False as values. The strings will be tested against a lowered
+        version of 's'.
+
+    :return: True or False for accepted strings, None otherwise.
+    """
+    if accepted_values is None:
+        accepted_values = _valid_boolean_strings
+    val = None
+    if type(s) in (str, unicode):
+        try:
+            val = accepted_values[s.lower()]
+        except KeyError:
+            pass
+    return val
+
+
 class UIFactory(object):
     """UI abstraction.
 
@@ -156,15 +192,16 @@
         self.stdout = stdout or sys.stdout
         self.stderr = stderr or sys.stderr
 
+    _accepted_boolean_strings = dict(y=True, n=False, yes=True, no=False)
+
     def get_boolean(self, prompt):
         while True:
             self.prompt(prompt + "? [y/n]: ")
             line = self.stdin.readline()
-            line = line.lower()
-            if line in ('y\n', 'yes\n'):
-                return True
-            if line in ('n\n', 'no\n'):
-                return False
+            line = line.rstrip('\n')
+            val = bool_from_string(line, self._accepted_boolean_strings)
+            if val is not None:
+                return val
 
     def get_non_echoed_password(self):
         isatty = getattr(self.stdin, 'isatty', None)