Variables cleanup: EnumVariable

Part 2 of a series, updating the EnumVariable implementation,
tests and docstrings.  While this is a small change, it looks
bigger in the diff, due to the conversion of a series of lambdas
to inner fuctions (fixing a pylint complaint)

Signed-off-by: Mats Wichmann <mats@linux.com>

3 files changed, 91 insertions, 59 deletions

diff --git a/SCons/Variables/EnumVariable.py b/SCons/Variables/EnumVariable.py
index 1a4f3fb..d13e9a9 100644
--- a/SCons/Variables/EnumVariable.py
+++ b/SCons/Variables/EnumVariable.py
@@ -38,12 +38,12 @@ Usage example::
             ignorecase=2,
         )
     )
-    ...
+    env = Environment(variables=opts)
     if env['debug'] == 'full':
-    ...
+        ...
 """
 
-from typing import Tuple, Callable
+from typing import Callable, List, Optional, Tuple
 
 import SCons.Errors
 
@@ -51,57 +51,97 @@ __all__ = ['EnumVariable',]
 
 
 def _validator(key, val, env, vals) -> None:
-    if val not in vals:
-        raise SCons.Errors.UserError(
-            'Invalid value for option %s: %s.  Valid values are: %s' % (key, val, vals))
-
+    """Validate that val is in vals.
 
-def EnumVariable(key, help, default, allowed_values, map={}, ignorecase: int=0) -> Tuple[str, str, str, Callable, Callable]:
+    Usable as the base for :class:`EnumVariable` validators.
+    """
+    if val not in vals:
+        msg = (
+            f"Invalid value for enum variable {key!r}: {val!r}. "
+            f"Valid values are: {vals}"
+        )
+        raise SCons.Errors.UserError(msg) from None
+
+
+# lint: W0622: Redefining built-in 'help' (redefined-builtin)
+# lint:  W0622: Redefining built-in 'map' (redefined-builtin)
+def EnumVariable(
+    key,
+    help: str,
+    default: str,
+    allowed_values: List[str],
+    map: Optional[dict] = None,
+    ignorecase: int = 0,
+) -> Tuple[str, str, str, Callable, Callable]:
     """Return a tuple describing an enumaration SCons Variable.
 
-    The input parameters describe an option with only certain values
-    allowed. Returns A tuple including an appropriate converter and
-    validator. The result is usable as input to :meth:`Add`.
-
-    *key* and *default* are passed directly on to :meth:`Add`.
-
-    *help* is the descriptive part of the help text,
-    and will have the allowed values automatically appended.
+    The input parameters describe a variable with only predefined values
+    allowed.  The value of *ignorecase* defines the behavior of the
+    validator and converter: if ``0``, the validator/converter are
+    case-sensitive; if ``1``, the validator/converter are case-insensitive;
+    if ``2``, the validator/converter are case-insensitive and the
+    converted value will always be lower-case.
+
+    Arguments:
+       key: variable name, passed directly through to the return tuple.
+       default: default values, passed directly through to the return tuple.
+       help: descriptive part of the help text,
+          will have the allowed values automatically appended.
+       allowed_values: list of the allowed values for this variable.
+       map: optional dictionary which may be used for converting the
+          input value into canonical values (e.g. for aliases).
+       ignorecase: defines the behavior of the validator and converter.
+       validator: callback function to test whether the value is in the
+          list of allowed values.
+       converter: callback function to convert input values according to
+          the given *map*-dictionary. Unmapped input values are returned
+          unchanged.
+
+    Returns:
+       A tuple including an appropriate converter and validator.
+       The result is usable as input to :meth:`~SCons.Variables.Variables.Add`.
+       and :meth:`~SCons.Variables.Variables.AddVariables`.
+    """
+    # these are all inner functions so they can access EnumVariable locals.
+    def validator_rcase(key, val, env):
+        """Case-respecting validator."""
+        return _validator(key, val, env, allowed_values)
 
-    *allowed_values* is a list of strings, which are the allowed values
-    for this option.
+    def validator_icase(key, val, env):
+        """Case-ignoring validator."""
+        return _validator(key, val.lower(), env, allowed_values)
 
-    The *map*-dictionary may be used for converting the input value
-    into canonical values (e.g. for aliases).
+    def converter_rcase(val):
+        """Case-respecting converter."""
+        return map.get(val, val)
 
-    The value of *ignorecase* defines the behaviour of the validator:
+    def converter_icase(val):
+        """Case-ignoring converter."""
+        return map.get(val.lower(), val)
 
-        * 0: the validator/converter are case-sensitive.
-        * 1: the validator/converter are case-insensitive.
-        * 2: the validator/converter is case-insensitive and the
-          converted value will always be lower-case.
+    def converter_lcase(val):
+        """Case-lowering converter."""
+        return map.get(val.lower(), val).lower()
 
-    The *validator* tests whether the value is in the list of allowed values.
-    The *converter* converts input values according to the given
-    *map*-dictionary (unmapped input values are returned unchanged).
-    """
+    if map is None:
+        map = {}
+    help = f"{help} ({'|'.join(allowed_values)})"
 
-    help = '%s (%s)' % (help, '|'.join(allowed_values))
     # define validator
     if ignorecase:
-        validator = lambda key, val, env: \
-                    _validator(key, val.lower(), env, allowed_values)
+        validator = validator_icase
     else:
-        validator = lambda key, val, env: \
-                    _validator(key, val, env, allowed_values)
+        validator = validator_rcase
+
     # define converter
     if ignorecase == 2:
-        converter = lambda val: map.get(val.lower(), val).lower()
+        converter = converter_lcase
     elif ignorecase == 1:
-        converter = lambda val: map.get(val.lower(), val)
+        converter = converter_icase
     else:
-        converter = lambda val: map.get(val, val)
-    return (key, help, default, validator, converter)
+        converter = converter_rcase
+
+    return key, help, default, validator, converter
 
 # Local Variables:
 # tab-width:4
diff --git a/SCons/Variables/EnumVariableTests.py b/SCons/Variables/EnumVariableTests.py
index cc004f8..c4f3278 100644
--- a/SCons/Variables/EnumVariableTests.py
+++ b/SCons/Variables/EnumVariableTests.py
@@ -121,11 +121,11 @@ class EnumVariableTestCase(unittest.TestCase):
 
         for k, l in table.items():
             x = o0.converter(k)
-            assert x == l[0], "o0 got %s, expected %s" % (x, l[0])
+            assert x == l[0], f"o0 got {x}, expected {l[0]}"
             x = o1.converter(k)
-            assert x == l[1], "o1 got %s, expected %s" % (x, l[1])
+            assert x == l[1], f"o1 got {x}, expected {l[1]}"
             x = o2.converter(k)
-            assert x == l[2], "o2 got %s, expected %s" % (x, l[2])
+            assert x == l[2], f"o2 got {x}, expected {l[2]}"
 
     def test_validator(self) -> None:
         """Test the EnumVariable validator"""
@@ -157,13 +157,11 @@ class EnumVariableTestCase(unittest.TestCase):
             o.validator('X', v, {})
 
         def invalid(o, v) -> None:
-            caught = None
-            try:
+            with self.assertRaises(
+                SCons.Errors.UserError,
+                msg=f"did not catch expected UserError for o = {o.key}, v = {v}",
+            ):
                 o.validator('X', v, {})
-            except SCons.Errors.UserError:
-                caught = 1
-            assert caught, "did not catch expected UserError for o = %s, v = %s" % (o.key, v)
-
         table = {
             'one'   : [  valid,   valid,   valid],
             'One'   : [invalid,   valid,   valid],
diff --git a/test/Variables/EnumVariable.py b/test/Variables/EnumVariable.py
index 14a8bf3..111fff3 100644
--- a/test/Variables/EnumVariable.py
+++ b/test/Variables/EnumVariable.py
@@ -1,6 +1,8 @@
 #!/usr/bin/env python
 #
-# __COPYRIGHT__
+# MIT License
+#
+# Copyright The SCons Foundation
 #
 # Permission is hereby granted, free of charge, to any person obtaining
 # a copy of this software and associated documentation files (the
@@ -20,15 +22,11 @@
 # LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
 # OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
-#
-
-__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
 """
 Test the EnumVariable canned Variable type.
 """
 
-
 import TestSCons
 
 test = TestSCons.TestSCons()
@@ -39,8 +37,6 @@ def check(expect):
     result = test.stdout().split('\n')
     assert result[1:len(expect)+1] == expect, (result[1:len(expect)+1], expect)
 
-
-
 test.write(SConstruct_path, """\
 from SCons.Variables.EnumVariable import EnumVariable
 EV = EnumVariable
@@ -72,7 +68,6 @@ print(env['some'])
 Default(env.Alias('dummy', None))
 """)
 
-
 test.run(); check(['no', 'gtk', 'xaver'])
 
 test.run(arguments='debug=yes guilib=Motif some=xAVER')
@@ -82,24 +77,23 @@ test.run(arguments='debug=full guilib=KdE some=EiNs')
 check(['full', 'KdE', 'eins'])
 
 expect_stderr = """
-scons: *** Invalid value for option debug: FULL.  Valid values are: ('yes', 'no', 'full')
+scons: *** Invalid value for enum variable 'debug': 'FULL'. Valid values are: ('yes', 'no', 'full')
 """ + test.python_file_line(SConstruct_path, 21)
 
 test.run(arguments='debug=FULL', stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Invalid value for option guilib: irgendwas.  Valid values are: ('motif', 'gtk', 'kde')
+scons: *** Invalid value for enum variable 'guilib': 'irgendwas'. Valid values are: ('motif', 'gtk', 'kde')
 """ + test.python_file_line(SConstruct_path, 21)
 
 test.run(arguments='guilib=IrGeNdwas', stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Invalid value for option some: irgendwas.  Valid values are: ('xaver', 'eins')
+scons: *** Invalid value for enum variable 'some': 'irgendwas'. Valid values are: ('xaver', 'eins')
 """ + test.python_file_line(SConstruct_path, 21)
 
 test.run(arguments='some=IrGeNdwas', stderr=expect_stderr, status=2)
 
-
 test.pass_test()
 
 # Local Variables: