Variables cleanup: ListVariable

SCons/Variables/ListVariable.py

@@ -21,10 +21,10 @@
 # OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-"""Variable type for list Variables.
+"""Variable type for List Variables.
 
-A 'list' option may either be 'all', 'none' or a list of names
-separated by comma. After the option has been processed, the option
+A list variable may given as 'all', 'none' or a list of names
+separated by comma. After the variable has been processed, the variable
 value holds either the named list elements, all list elements or no
 list elements at all.
 
@@ -41,7 +41,7 @@
             elems=list_of_libs,
         )
     )
-    ...
+    env = Environment(variables=opts)
     for lib in list_of_libs:
         if lib in env['shared']:
             env.SharedObject(...)
@@ -53,14 +53,21 @@
 # since elements can occur twice.
 
 import collections
-from typing import Tuple, Callable
+from typing import Callable, List, Optional, Tuple, Union
 
 import SCons.Util
 
 __all__ = ['ListVariable',]
 
 
 class _ListVariable(collections.UserList):
+    """Internal class holding the data for a List Variable.
+
+    The initializer accepts two arguments, the list of actual values
+    given, and the list of allowable values. Not normally instantiated
+    by hand, but rather by the ListVariable converter function.
+    """
+
     def __init__(self, initlist=None, allowedElems=None) -> None:
         if initlist is None:
             initlist = []
@@ -88,19 +95,18 @@ def __lt__(self, other):
         return NotImplemented
 
     def __str__(self) -> str:
-        if not len(self):
+        if not self.data:
             return 'none'
         self.data.sort()
         if self.data == self.allowedElems:
             return 'all'
-        else:
-            return ','.join(self)
+        return ','.join(self)
 
     def prepare_to_store(self):
-        return self.__str__()
+        return str(self)
 
 def _converter(val, allowedElems, mapdict) -> _ListVariable:
-    """ """
+    """Convert list variables."""
     if val == 'none':
         val = []
     elif val == 'all':
@@ -111,39 +117,57 @@ def _converter(val, allowedElems, mapdict) -> _ListVariable:
         notAllowed = [v for v in val if v not in allowedElems]
         if notAllowed:
             raise ValueError(
-                "Invalid value(s) for option: %s" % ','.join(notAllowed)
+                f"Invalid value(s) for option: {','.join(notAllowed)}"
             )
     return _ListVariable(val, allowedElems)
 
 
 # def _validator(key, val, env) -> None:
 #     """ """
-#     # TODO: write validator for pgk list
+#     # TODO: write validator for list variable
 #     pass
 
 
-def ListVariable(key, help, default, names, map={}) -> Tuple[str, str, str, None, Callable]:
-    """Return a tuple describing a list SCons Variable.
-
-    The input parameters describe a 'list' option. Returns
-    a tuple including the correct converter and validator.
-    The result is usable for input to :meth:`Add`.
-
-    *help* will have text appended indicating the legal values
-    (not including any extra names from *map*).
-
-    *map* can be used to map alternative names to the ones in *names* -
-    that is, a form of alias.
-
-    A 'list' option may either be 'all', 'none' or a list of
-    names (separated by commas).
+# lint: W0622: Redefining built-in 'help' (redefined-builtin)
+# lint: W0622: Redefining built-in 'map' (redefined-builtin)
+def ListVariable(
+    key,
+    help: str,
+    default: Union[str, List[str]],
+    names: List[str],
+    map: Optional[dict] = None,
+) -> Tuple[str, str, str, None, Callable]:
+    """Return a tuple describing a list variable.
+
+    The input parameters describe a list variable, where the values
+    can be one or more from *names* plus the special values ``all``
+    and ``none``.
+
+    Arguments:
+       key: the name of the list variable.
+       help: the basic help message.  Will have text appended indicating
+          the allowable values (not including any extra names from *map*).
+       default: the default value(s) for the list variable. Can be
+          given as string (possibly comma-separated), or as a list of strings.
+          ``all`` or ``none`` are allowed as *default*.
+       names: the allowable values. Must be a list of strings.
+       map: optional dictionary to map alternative names to the ones in
+          *names*, providing a form of alias. The converter will make
+          the replacement, names from *map* are not stored and will
+          not appear in the help message.
+
+    Returns:
+       A tuple including the correct converter and validator.  The
+       result is usable as input to :meth:`~SCons.Variables.Variables.Add`.
     """
-    names_str = 'allowed names: %s' % ' '.join(names)
+    if map is None:
+        map = {}
+    names_str = f"allowed names: {' '.join(names)}"
     if SCons.Util.is_List(default):
         default = ','.join(default)
     help = '\n    '.join(
         (help, '(all|none|comma-separated list of names)', names_str))
-    return (key, help, default, None, lambda val: _converter(val, names, map))
+    return key, help, default, None, lambda val: _converter(val, names, map)
 
 # Local Variables:
 # tab-width:4

SCons/Variables/ListVariableTests.py

@@ -101,12 +101,8 @@ def test_converter(self) -> None:
         x = o.converter('three,ONE,TWO')
         assert str(x) == 'all', x
 
-        caught = None
-        try:
+        with self.assertRaises(ValueError):
             x = o.converter('no_match')
-        except ValueError:
-            caught = 1
-        assert caught, "did not catch expected ValueError"
 
     def test_copy(self) -> None:
         """Test copying a ListVariable like an Environment would"""

test/Variables/ListVariable.py

@@ -149,8 +149,6 @@ def check(expect):
 
 test.run(arguments='shared=foo,x11,,,bar', stderr=expect_stderr, status=2)
 
-
-
 test.write('SConstruct', """
 from SCons.Variables import ListVariable
 
@@ -174,8 +172,6 @@ def check(expect):
 scons: Nothing to be done for `dummy'.
 """))
 
-
-
 test.pass_test()
 
 # Local Variables: