Variables cleanup: PackageVariable

SCons/Variables/PackageVariable.py

@@ -42,15 +42,16 @@
             default='yes'
         )
     )
-    ...
-    if env['x11'] == True:
+    env = Environment(variables=opts)
+    if env['x11'] is True:
         dir = ...  # search X11 in some standard places ...
         env['x11'] = dir
     if env['x11']:
         ...  # build with x11 ...
 """
 
-from typing import Tuple, Callable
+import os
+from typing import Callable, Optional, Tuple
 
 import SCons.Errors
 
@@ -60,7 +61,12 @@
 DISABLE_STRINGS = ('0', 'no',  'false', 'off', 'disable')
 
 def _converter(val):
-    """ """
+    """Convert package variables.
+
+    Returns True or False if one of the recognized truthy or falsy
+    values is seen, else return the value unchanged (expected to
+    be a path string).
+    """
     lval = val.lower()
     if lval in ENABLE_STRINGS:
         return True
@@ -70,35 +76,45 @@ def _converter(val):
 
 
 def _validator(key, val, env, searchfunc) -> None:
-    """ """
-    # NB: searchfunc is currently undocumented and unsupported
-    # TODO write validator, check for path
-    import os
+    """Validate package variable for valid path.
 
+    Checks that if a path is given as the value, that pathname actually exists.
+    """
+    # NOTE: searchfunc is currently undocumented and unsupported
     if env[key] is True:
         if searchfunc:
             env[key] = searchfunc(key, val)
+            # TODO: need to check path, or be sure searchfunc raises.
     elif env[key] and not os.path.exists(val):
-        raise SCons.Errors.UserError(
-            'Path does not exist for option %s: %s' % (key, val))
+        msg = f'Path does not exist for variable {key!r}: {val!r}'
+        raise SCons.Errors.UserError(msg) from None
 
 
-def PackageVariable(key, help, default, searchfunc=None) -> Tuple[str, str, str, Callable, Callable]:
+# lint: W0622: Redefining built-in 'help' (redefined-builtin)
+def PackageVariable(
+    key: str, help: str, default, searchfunc: Optional[Callable] = None
+) -> Tuple[str, str, str, Callable, Callable]:
     """Return a tuple describing a package list SCons Variable.
 
-    The input parameters describe a 'package list' option. Returns
-    a tuple including the correct converter and validator appended.
-    The result is usable as input to :meth:`Add` .
+    The input parameters describe a 'package list' variable. Returns
+    a tuple with the correct converter and validator appended.
+    The result is usable as input to :meth:`~SCons.Variables.Variables.Add`.
 
-    A 'package list' option may either be 'all', 'none' or a pathname
-    string. This information is appended to *help*.
+    A 'package list' variable may either be a truthy string from
+    :const:`ENABLE_STRINGS`, a falsy string from
+    :const:`DISABLE_STRINGS`, or a pathname string.
+    This information is appended to *help* using only one string
+    each for truthy/falsy.
     """
     # NB: searchfunc is currently undocumented and unsupported
-    help = '\n    '.join(
-        (help, '( yes | no | /path/to/%s )' % key))
-    return (key, help, default,
-            lambda k, v, e: _validator(k, v, e, searchfunc),
-            _converter)
+    help = '\n    '.join((help, f'( yes | no | /path/to/{key} )'))
+    return (
+        key,
+        help,
+        default,
+        lambda k, v, e: _validator(k, v, e, searchfunc),
+        _converter,
+    )
 
 # Local Variables:
 # tab-width:4

SCons/Variables/PackageVariableTests.py

@@ -32,19 +32,19 @@ class PackageVariableTestCase(unittest.TestCase):
     def test_PackageVariable(self) -> None:
         """Test PackageVariable creation"""
         opts = SCons.Variables.Variables()
-        opts.Add(SCons.Variables.PackageVariable('test', 'test option help', '/default/path'))
+        opts.Add(SCons.Variables.PackageVariable('test', 'test build variable help', '/default/path'))
 
         o = opts.options[0]
         assert o.key == 'test', o.key
-        assert o.help == 'test option help\n    ( yes | no | /path/to/test )', repr(o.help)
+        assert o.help == 'test build variable help\n    ( yes | no | /path/to/test )', repr(o.help)
         assert o.default == '/default/path', o.default
         assert o.validator is not None, o.validator
         assert o.converter is not None, o.converter
 
     def test_converter(self) -> None:
         """Test the PackageVariable converter"""
         opts = SCons.Variables.Variables()
-        opts.Add(SCons.Variables.PackageVariable('test', 'test option help', '/default/path'))
+        opts.Add(SCons.Variables.PackageVariable('test', 'test build variable help', '/default/path'))
 
         o = opts.options[0]
 
@@ -64,11 +64,11 @@ def test_converter(self) -> None:
 
         for t in true_values:
             x = o.converter(t)
-            assert x, "converter returned false for '%s'" % t
+            assert x, f"converter returned False for {t!r}"
 
         for f in false_values:
             x = o.converter(f)
-            assert not x, "converter returned true for '%s'" % f
+            assert not x, f"converter returned True for {f!r}"
 
         x = o.converter('/explicit/path')
         assert x == '/explicit/path', x
@@ -85,7 +85,7 @@ def test_converter(self) -> None:
     def test_validator(self) -> None:
         """Test the PackageVariable validator"""
         opts = SCons.Variables.Variables()
-        opts.Add(SCons.Variables.PackageVariable('test', 'test option help', '/default/path'))
+        opts.Add(SCons.Variables.PackageVariable('test', 'test build variable help', '/default/path'))
 
         test = TestCmd.TestCmd(workdir='')
         test.write('exists', 'exists\n')
@@ -101,12 +101,8 @@ def test_validator(self) -> None:
         o.validator('T', '/path', env)
         o.validator('X', exists, env)
 
-        caught = None
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', does_not_exist, env)
-        except SCons.Errors.UserError:
-            caught = 1
-        assert caught, "did not catch expected UserError"
 
 
 if __name__ == "__main__":

test/Variables/PackageVariable.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,9 +22,6 @@
 # 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 PackageVariable canned Variable type.
@@ -39,8 +38,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.PackageVariable import PackageVariable
 PV = PackageVariable
@@ -75,13 +72,11 @@ def check(expect):
 check([test.workpath()])
 
 expect_stderr = """
-scons: *** Path does not exist for option x11: /non/existing/path/
+scons: *** Path does not exist for variable 'x11': '/non/existing/path/'
 """ + test.python_file_line(SConstruct_path, 14)
 
 test.run(arguments='x11=/non/existing/path/', stderr=expect_stderr, status=2)
 
-
-
 test.pass_test()
 
 # Local Variables: