Variables cleanup: PathVariable

Part 4 of a series, updating the PathVariable implementation,
tests and docstrings.

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

3 files changed, 112 insertions, 135 deletions

diff --git a/SCons/Variables/PathVariable.py b/SCons/Variables/PathVariable.py
index eb471a2..6ea4e6b 100644
--- a/SCons/Variables/PathVariable.py
+++ b/SCons/Variables/PathVariable.py
@@ -26,10 +26,10 @@
 To be used whenever a user-specified path override setting should be allowed.
 
 Arguments to PathVariable are:
-  * *key* - name of this option on the command line (e.g. "prefix")
-  * *help* - help string for option
-  * *default* - default value for this option
-  * *validator* - [optional] validator for option value.  Predefined are:
+  * *key* - name of this variable on the command line (e.g. "prefix")
+  * *help* - help string for variable
+  * *default* - default value for this variable
+  * *validator* - [optional] validator for variable value.  Predefined are:
 
     * *PathAccept* - accepts any path setting; no validation
     * *PathIsDir* - path must be an existing directory
@@ -40,8 +40,8 @@ Arguments to PathVariable are:
   The *validator* is a function that is called and which should return
   True or False to indicate if the path is valid.  The arguments
   to the validator function are: (*key*, *val*, *env*).  *key* is the
-  name of the option, *val* is the path specified for the option,
-  and *env* is the environment to which the Options have been added.
+  name of the variable, *val* is the path specified for the variable,
+  and *env* is the environment to which the Variables have been added.
 
 Usage example::
 
@@ -74,71 +74,83 @@ Usage example::
 
 import os
 import os.path
-from typing import Tuple, Callable
+from typing import Callable, Optional, Tuple
 
 import SCons.Errors
+import SCons.Util
 
 __all__ = ['PathVariable',]
 
 class _PathVariableClass:
+    """Class implementing path variables.
+
+    This class exists mainly to expose the validators without code having
+    to import the names: they will appear as methods of ``PathVariable``,
+    a statically created instance of this class, which is placed in
+    the SConscript namespace.
+
+    Instances are callable to produce a suitable variable tuple.
+    """
 
     @staticmethod
     def PathAccept(key, val, env) -> None:
-        """Accepts any path, no checking done."""
-        pass
+        """Validate path with no checking."""
+        return
 
     @staticmethod
     def PathIsDir(key, val, env) -> None:
-        """Validator to check if Path is a directory."""
-        if not os.path.isdir(val):
-            if os.path.isfile(val):
-                m = 'Directory path for option %s is a file: %s'
-            else:
-                m = 'Directory path for option %s does not exist: %s'
-            raise SCons.Errors.UserError(m % (key, val))
+        """Validate path is a directory."""
+        if os.path.isdir(val):
+            return
+        if os.path.isfile(val):
+            msg = f'Directory path for variable {key!r} is a file: {val}'
+        else:
+            msg = f'Directory path for variable {key!r} does not exist: {val}'
+        raise SCons.Errors.UserError(msg)
 
     @staticmethod
     def PathIsDirCreate(key, val, env) -> None:
-        """Validator to check if Path is a directory,
-           creating it if it does not exist."""
+        """Validate path is a directory, creating if needed."""
+        if os.path.isdir(val):
+            return
         try:
             os.makedirs(val, exist_ok=True)
-        except FileExistsError:
-            m = 'Path for option %s is a file, not a directory: %s'
-            raise SCons.Errors.UserError(m % (key, val))
-        except PermissionError:
-            m = 'Path for option %s could not be created: %s'
-            raise SCons.Errors.UserError(m % (key, val))
-        except OSError:
-            m = 'Path for option %s could not be created: %s'
-            raise SCons.Errors.UserError(m % (key, val))
+        except FileExistsError as exc:
+            msg = f'Path for variable {key!r} is a file, not a directory: {val}'
+            raise SCons.Errors.UserError(msg) from exc
+        except (PermissionError, OSError) as exc:
+            msg = f'Path for variable {key!r} could not be created: {val}'
+            raise SCons.Errors.UserError(msg) from exc
 
     @staticmethod
     def PathIsFile(key, val, env) -> None:
-        """Validator to check if Path is a file"""
+        """Validate path is a file."""
         if not os.path.isfile(val):
             if os.path.isdir(val):
-                m = 'File path for option %s is a directory: %s'
+                msg = f'File path for variable {key!r} is a directory: {val}'
             else:
-                m = 'File path for option %s does not exist: %s'
-            raise SCons.Errors.UserError(m % (key, val))
+                msg = f'File path for variable {key!r} does not exist: {val}'
+            raise SCons.Errors.UserError(msg)
 
     @staticmethod
     def PathExists(key, val, env) -> None:
-        """Validator to check if Path exists"""
+        """Validate path exists."""
         if not os.path.exists(val):
-            m = 'Path for option %s does not exist: %s'
-            raise SCons.Errors.UserError(m % (key, val))
+            msg = f'Path for variable {key!r} does not exist: {val}'
+            raise SCons.Errors.UserError(msg)
 
-    def __call__(self, key, help, default, validator=None) -> Tuple[str, str, str, Callable, None]:
+    # lint: W0622: Redefining built-in 'help' (redefined-builtin)
+    def __call__(
+        self, key, help: str, default, validator: Optional[Callable] = None
+    ) -> Tuple[str, str, str, Callable, None]:
         """Return a tuple describing a path list SCons Variable.
 
-        The input parameters describe a 'path list' option. Returns
+        The input parameters describe a 'path list' variable. Returns
         a tuple with the correct converter and validator appended. The
         result is usable for input to :meth:`Add`.
 
-        The *default* option specifies the default path to use if the
-        user does not specify an override with this option.
+        The *default* parameter specifies the default path to use if the
+        user does not specify an override with this variable.
 
         *validator* is a validator, see this file for examples
         """
@@ -146,9 +158,9 @@ class _PathVariableClass:
             validator = self.PathExists
 
         if SCons.Util.is_List(key) or SCons.Util.is_Tuple(key):
-            helpmsg = '%s ( /path/to/%s )' % (help, key[0])
+            helpmsg = f'{help} ( /path/to/{key[0]} )'
         else:
-            helpmsg = '%s ( /path/to/%s )' % (help, key)
+            helpmsg = f'{help} ( /path/to/{key} )'
         return (key, helpmsg, default, validator, None)
 
 
diff --git a/SCons/Variables/PathVariableTests.py b/SCons/Variables/PathVariableTests.py
index aacfc9e..efc75f1 100644
--- a/SCons/Variables/PathVariableTests.py
+++ b/SCons/Variables/PathVariableTests.py
@@ -28,18 +28,19 @@ import SCons.Errors
 import SCons.Variables
 
 import TestCmd
+from TestCmd import IS_WINDOWS
 
 class PathVariableTestCase(unittest.TestCase):
     def test_PathVariable(self) -> None:
         """Test PathVariable creation"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path'))
 
         o = opts.options[0]
         assert o.key == 'test', o.key
-        assert o.help == 'test option help ( /path/to/test )', repr(o.help)
+        assert o.help == 'test build variable help ( /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 None, o.converter
@@ -48,7 +49,7 @@ class PathVariableTestCase(unittest.TestCase):
         """Test the PathExists validator"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path',
                                           SCons.Variables.PathVariable.PathExists))
 
@@ -56,22 +57,19 @@ class PathVariableTestCase(unittest.TestCase):
         test.write('exists', 'exists\n')
 
         o = opts.options[0]
-
         o.validator('X', test.workpath('exists'), {})
 
         dne = test.workpath('does_not_exist')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', dne, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'Path for option X does not exist: %s' % dne, e
-        except:
-            raise Exception("did not catch expected UserError")
+        e = cm.exception
+        self.assertEqual(str(e), f"Path for variable 'X' does not exist: {dne}")
 
     def test_PathIsDir(self):
         """Test the PathIsDir validator"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path',
                                           SCons.Variables.PathVariable.PathIsDir))
 
@@ -80,30 +78,25 @@ class PathVariableTestCase(unittest.TestCase):
         test.write('file', "file\n")
 
         o = opts.options[0]
-
         o.validator('X', test.workpath('dir'), {})
 
         f = test.workpath('file')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', f, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'Directory path for option X is a file: %s' % f, e
-        except:
-            raise Exception("did not catch expected UserError")
+        e = cm.exception
+        self.assertEqual(str(e), f"Directory path for variable 'X' is a file: {f}")
 
         dne = test.workpath('does_not_exist')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', dne, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'Directory path for option X does not exist: %s' % dne, e
-        except Exception as e:
-            raise Exception("did not catch expected UserError") from e
+        e = cm.exception
+        self.assertEqual(str(e), f"Directory path for variable 'X' does not exist: {dne}")
 
     def test_PathIsDirCreate(self):
         """Test the PathIsDirCreate validator"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path',
                                           SCons.Variables.PathVariable.PathIsDirCreate))
 
@@ -117,26 +110,26 @@ class PathVariableTestCase(unittest.TestCase):
         assert os.path.isdir(d)
 
         f = test.workpath('file')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', f, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'Path for option X is a file, not a directory: %s' % f, e
-        except Exception as e:
-            raise Exception("did not catch expected UserError") from e
+        e = cm.exception
+        self.assertEqual(str(e), f"Path for variable 'X' is a file, not a directory: {f}")
 
-        f = '/yyy/zzz'  # this not exists and should fail to create
-        try:
+        # pick a directory path that can't be mkdir'd
+        if IS_WINDOWS:
+            f = r'\\noserver\noshare\yyy\zzz'
+        else:
+            f = '/yyy/zzz'
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', f, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'Path for option X could not be created: %s' % f, e
-        except Exception as e:
-            raise Exception("did not catch expected UserError") from e
+        e = cm.exception
+        self.assertEqual(str(e), f"Path for variable 'X' could not be created: {f}")
 
     def test_PathIsFile(self):
         """Test the PathIsFile validator"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path',
                                           SCons.Variables.PathVariable.PathIsFile))
 
@@ -145,30 +138,25 @@ class PathVariableTestCase(unittest.TestCase):
         test.write('file', "file\n")
 
         o = opts.options[0]
-
         o.validator('X', test.workpath('file'), {})
 
         d = test.workpath('d')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', d, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'File path for option X does not exist: %s' % d, e
-        except:
-            raise Exception("did not catch expected UserError")
+        e = cm.exception
+        self.assertEqual(str(e), f"File path for variable 'X' does not exist: {d}")
 
         dne = test.workpath('does_not_exist')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', dne, {})
-        except SCons.Errors.UserError as e:
-            assert str(e) == 'File path for option X does not exist: %s' % dne, e
-        except:
-            raise Exception("did not catch expected UserError")
+        e = cm.exception
+        self.assertEqual(str(e), f"File path for variable 'X' does not exist: {dne}")
 
     def test_PathAccept(self) -> None:
         """Test the PathAccept validator"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test build variable help',
                                           '/default/path',
                                           SCons.Variables.PathVariable.PathAccept))
 
@@ -177,7 +165,6 @@ class PathVariableTestCase(unittest.TestCase):
         test.write('file', "file\n")
 
         o = opts.options[0]
-
         o.validator('X', test.workpath('file'), {})
 
         d = test.workpath('d')
@@ -190,44 +177,37 @@ class PathVariableTestCase(unittest.TestCase):
         """Test the PathVariable validator argument"""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
-                                          'test option help',
+                                          'test variable help',
                                           '/default/path'))
 
         test = TestCmd.TestCmd(workdir='')
         test.write('exists', 'exists\n')
 
         o = opts.options[0]
-
         o.validator('X', test.workpath('exists'), {})
 
         dne = test.workpath('does_not_exist')
-        try:
+        with self.assertRaises(SCons.Errors.UserError) as cm:
             o.validator('X', dne, {})
-        except SCons.Errors.UserError as e:
-            expect = 'Path for option X does not exist: %s' % dne
-            assert str(e) == expect, e
-        else:
-            raise Exception("did not catch expected UserError")
+        e = cm.exception
+        self.assertEqual(str(e), f"Path for variable 'X' does not exist: {dne}")
+
+        class ValidatorError(Exception):
+            pass
 
         def my_validator(key, val, env):
-            raise Exception("my_validator() got called for %s, %s!" % (key, val))
+            raise ValidatorError(f"my_validator() got called for {key!r}, {val}!")
 
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test2',
                                           'more help',
                                           '/default/path/again',
                                           my_validator))
-
         o = opts.options[0]
-
-        try:
+        with self.assertRaises(ValidatorError) as cm:
             o.validator('Y', 'value', {})
-        except Exception as e:
-            assert str(e) == 'my_validator() got called for Y, value!', e
-        else:
-            raise Exception("did not catch expected exception from my_validator()")
-
-
+        e = cm.exception
+        self.assertEqual(str(e), f"my_validator() got called for 'Y', value!")
 
 
 if __name__ == "__main__":
diff --git a/test/Variables/PathVariable.py b/test/Variables/PathVariable.py
index ea5e15f..265f48f 100644
--- a/test/Variables/PathVariable.py
+++ b/test/Variables/PathVariable.py
@@ -24,7 +24,7 @@
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 """
-Test the PathVariable canned option type, with tests for its 
+Test the PathVariable canned variable type, with tests for its
 various canned validators.
 """
 
@@ -58,7 +58,7 @@ opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
     PathVariable('qtdir', 'where the root of Qt is installed', qtdir),
     PV('qt_libraries', 'where the Qt library is installed', r'%s'),
-    )
+)
 
 DefaultEnvironment(tools=[])  # test speedup
 env = Environment(variables=opts)
@@ -95,24 +95,21 @@ qtpath = os.path.join(workpath, 'non', 'existing', 'path')
 SConstruct_file_line = test.python_file_line(test.workpath('SConstruct'), 15)[:-1]
 
 expect_stderr = """
-scons: *** Path for option qtdir does not exist: %(qtpath)s
+scons: *** Path for variable 'qtdir' does not exist: %(qtpath)s
 %(SConstruct_file_line)s
 """ % locals()
 
 test.run(arguments=['qtdir=%s' % qtpath], stderr=expect_stderr, status=2)
 
 expect_stderr = """
-scons: *** Path for option qt_libraries does not exist: %(qtpath)s
+scons: *** Path for variable 'qt_libraries' does not exist: %(qtpath)s
 %(SConstruct_file_line)s
 """ % locals()
 
 test.run(arguments=['qt_libraries=%s' % qtpath], stderr=expect_stderr, status=2)
 
-
-
 default_file = test.workpath('default_file')
 default_subdir = test.workpath('default_subdir')
-
 existing_subdir = test.workpath('existing_subdir')
 test.subdir(existing_subdir)
 
@@ -122,13 +119,11 @@ test.write(existing_file, "existing_file\n")
 non_existing_subdir = test.workpath('non_existing_subdir')
 non_existing_file = test.workpath('non_existing_file')
 
-
-
 test.write('SConstruct', """\
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
     PathVariable('X', 'X variable', r'%s', validator=PathVariable.PathAccept),
-    )
+)
 
 DefaultEnvironment(tools=[])  # test speedup
 env = Environment(variables=opts)
@@ -156,13 +151,11 @@ check([non_existing_subdir])
 test.must_not_exist(non_existing_file)
 test.must_not_exist(non_existing_subdir)
 
-
-
 test.write(SConstruct_path, """\
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
     PathVariable('X', 'X variable', r'%s', validator=PathVariable.PathIsFile),
-    )
+)
 
 DefaultEnvironment(tools=[])  # test speedup
 env = Environment(variables=opts)
@@ -175,19 +168,18 @@ Default(env.Alias('dummy', None))
 SConstruct_file_line = test.python_file_line(test.workpath('SConstruct'), 7)[:-1]
 
 expect_stderr = """
-scons: *** File path for option X does not exist: %(default_file)s
+scons: *** File path for variable 'X' does not exist: %(default_file)s
 %(SConstruct_file_line)s
 """ % locals()
 
 test.run(status=2, stderr=expect_stderr)
 
 test.write(default_file, "default_file\n")
-
 test.run()
 check([default_file])
 
 expect_stderr = """
-scons: *** File path for option X is a directory: %(existing_subdir)s
+scons: *** File path for variable 'X' is a directory: %(existing_subdir)s
 %(SConstruct_file_line)s
 """ % locals()
 
@@ -197,19 +189,17 @@ test.run(arguments=['X=%s' % existing_file])
 check([existing_file])
 
 expect_stderr = """
-scons: *** File path for option X does not exist: %(non_existing_file)s
+scons: *** File path for variable 'X' does not exist: %(non_existing_file)s
 %(SConstruct_file_line)s
 """ % locals()
 
 test.run(arguments=['X=%s' % non_existing_file], status=2, stderr=expect_stderr)
 
-
-
 test.write('SConstruct', """\
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
     PathVariable('X', 'X variable', r'%s', validator=PathVariable.PathIsDir),
-    )
+)
 
 DefaultEnvironment(tools=[])  # test speedup
 env = Environment(variables=opts)
@@ -220,19 +210,18 @@ Default(env.Alias('dummy', None))
 """ % default_subdir)
 
 expect_stderr = """
-scons: *** Directory path for option X does not exist: %(default_subdir)s
+scons: *** Directory path for variable 'X' does not exist: %(default_subdir)s
 %(SConstruct_file_line)s
 """ % locals()
 
 test.run(status=2, stderr=expect_stderr)
 
 test.subdir(default_subdir)
-
 test.run()
 check([default_subdir])
 
 expect_stderr = """
-scons: *** Directory path for option X is a file: %(existing_file)s
+scons: *** Directory path for variable 'X' is a file: %(existing_file)s
 %(SConstruct_file_line)s
 """ % locals()
 
@@ -244,7 +233,7 @@ test.run(arguments=['X=%s' % existing_subdir])
 check([existing_subdir])
 
 expect_stderr = """
-scons: *** Directory path for option X does not exist: %(non_existing_subdir)s
+scons: *** Directory path for variable 'X' does not exist: %(non_existing_subdir)s
 %(SConstruct_file_line)s
 """ % locals()
 
@@ -252,13 +241,11 @@ test.run(arguments=['X=%s' % non_existing_subdir],
          status=2,
          stderr=expect_stderr)
 
-
-
 test.write('SConstruct', """\
 opts = Variables(args=ARGUMENTS)
 opts.AddVariables(
     PathVariable('X', 'X variable', r'%s', validator=PathVariable.PathIsDirCreate),
-    )
+)
 
 DefaultEnvironment(tools=[])  # test speedup
 env = Environment(variables=opts)
@@ -272,7 +259,7 @@ test.run()
 check([default_subdir])
 
 expect_stderr = """
-scons: *** Path for option X is a file, not a directory: %(existing_file)s
+scons: *** Path for variable 'X' is a file, not a directory: %(existing_file)s
 %(SConstruct_file_line)s
 """ % locals()
 
@@ -286,8 +273,6 @@ check([non_existing_subdir])
 
 test.must_exist(non_existing_subdir)
 
-
-
 test.pass_test()
 
 # Local Variables: