Fixed issue where run_pyscript failed if the script's filename had a space

kmvanbrunt · kmvanbrunt · commit 99ec5265e2e8 · 2019-07-23T21:00:32.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
     * Fixed bug where multiline commands were having leading and ending spaces stripped. This would mess up quoted
     strings that crossed multiple lines.
     * Fixed a bug when appending to the clipboard where contents were in reverse order
+    * Fixed issue where run_pyscript failed if the script's filename had a space
 * Enhancements
     * Greatly simplified using argparse-based tab completion. The new interface is a complete overhaul that breaks
     the previous way of specifying completion and choices functions. See header of [argparse_custom.py](https://github.com/python-cmd2/cmd2/blob/master/cmd2/argparse_custom.py)
diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py
@@ -3245,42 +3245,55 @@ def _restore_cmd2_env(self, cmd2_env: _SavedCmd2Env) -> None:
     py_parser.add_argument('command', nargs=argparse.OPTIONAL, help="command to run")
     py_parser.add_argument('remainder', nargs=argparse.REMAINDER, help="remainder of command")
 
+    # This is a hidden flag for telling do_py to run a pyscript. It is intended only to be used by run_pyscript
+    # after it sets up sys.argv for the script being run. When this flag is present, it takes precedence over all
+    # other arguments. run_pyscript uses this method instead of "py run('file')" because file names with spaces cause
+    # issues with our parser, which isn't meant to parse Python statements.
+    py_parser.add_argument('--pyscript', help=argparse.SUPPRESS)
+
     # Preserve quotes since we are passing these strings to Python
     @with_argparser(py_parser, preserve_quotes=True)
-    def do_py(self, args: argparse.Namespace) -> bool:
-        """Invoke Python command or shell"""
+    def do_py(self, args: argparse.Namespace) -> Optional[bool]:
+        """
+        Enter an interactive Python shell
+        :return: True if running of commands should stop
+        """
         from .py_bridge import PyBridge
         if self._in_py:
             err = "Recursively entering interactive Python consoles is not allowed."
             self.perror(err)
-            return False
+            return
 
         py_bridge = PyBridge(self)
+        py_code_to_run = ''
+
+        # Handle case where we were called by run_pyscript
+        if args.pyscript:
+            py_code_to_run = 'run({!r})'.format(args.pyscript)
+
+        elif args.command:
+            py_code_to_run = args.command
+            if args.remainder:
+                py_code_to_run += ' ' + ' '.join(args.remainder)
+
+            # Set cmd_echo to True so PyBridge statements like: py app('help')
+            # run at the command line will print their output.
+            py_bridge.cmd_echo = True
 
         try:
             self._in_py = True
 
-            # Support the run command even if called prior to invoking an interactive interpreter
             def py_run(filename: str):
                 """Run a Python script file in the interactive console.
-                :param filename: filename of *.py script file to run
+                :param filename: filename of script file to run
                 """
                 expanded_filename = os.path.expanduser(filename)
 
-                if not expanded_filename.endswith('.py'):
-                    self.pwarning("'{}' does not have a .py extension".format(expanded_filename))
-                    selection = self.select('Yes No', 'Continue to try to run it as a Python script? ')
-                    if selection != 'Yes':
-                        return
-
-                # cmd_echo defaults to False for scripts. The user can always toggle this value in their script.
-                py_bridge.cmd_echo = False
-
                 try:
                     with open(expanded_filename) as f:
                         interp.runcode(f.read())
                 except OSError as ex:
-                    self.pexcept("Error opening script file '{}': {}".format(expanded_filename, ex))
+                    self.pexcept("Error reading script file '{}': {}".format(expanded_filename, ex))
 
             def py_quit():
                 """Function callable from the interactive Python console to exit that environment"""
@@ -3301,24 +3314,16 @@ def py_quit():
             interp = InteractiveConsole(locals=localvars)
             interp.runcode('import sys, os;sys.path.insert(0, os.getcwd())')
 
-            # Check if the user is running a Python statement on the command line
-            if args.command:
-                full_command = args.command
-                if args.remainder:
-                    full_command += ' ' + ' '.join(args.remainder)
-
-                # Set cmd_echo to True so PyBridge statements like: py app('help')
-                # run at the command line will print their output.
-                py_bridge.cmd_echo = True
-
+            # Check if we are running Python code
+            if py_code_to_run:
                 # noinspection PyBroadException
                 try:
-                    interp.runcode(full_command)
+                    interp.runcode(py_code_to_run)
                 except BaseException:
-                    # We don't care about any exception that happened in the interactive console
+                    # We don't care about any exception that happened in the Python code
                     pass
 
-            # If there are no args, then we will open an interactive Python shell
+            # Otherwise we will open an interactive Python shell
             else:
                 cprt = 'Type "help", "copyright", "credits" or "license" for more information.'
                 instructions = ('End with `Ctrl-D` (Unix) / `Ctrl-Z` (Windows), `quit()`, `exit()`.\n'
@@ -3360,21 +3365,31 @@ def py_quit():
                                      help='arguments to pass to script', completer_method=path_complete)
 
     @with_argparser(run_pyscript_parser)
-    def do_run_pyscript(self, args: argparse.Namespace) -> bool:
-        """Run a Python script file inside the console"""
-        script_path = os.path.expanduser(args.script_path)
+    def do_run_pyscript(self, args: argparse.Namespace) -> Optional[bool]:
+        """
+        Run a Python script file inside the console
+         :return: True if running of commands should stop
+        """
+        # Expand ~ before placing this path in sys.argv just as a shell would
+        args.script_path = os.path.expanduser(args.script_path)
+
+        # Add some protection against accidentally running a non-Python file. The happens when users
+        # mix up run_script and run_pyscript.
+        if not args.script_path.endswith('.py'):
+            self.pwarning("'{}' does not have a .py extension".format(args.script_path))
+            selection = self.select('Yes No', 'Continue to try to run it as a Python script? ')
+            if selection != 'Yes':
+                return
+
         py_return = False
 
         # Save current command line arguments
         orig_args = sys.argv
 
         try:
             # Overwrite sys.argv to allow the script to take command line arguments
-            sys.argv = [script_path] + args.script_arguments
-
-            # Run the script - use repr formatting to escape things which
-            # need to be escaped to prevent issues on Windows
-            py_return = self.do_py("run({!r})".format(script_path))
+            sys.argv = [args.script_path] + args.script_arguments
+            py_return = self.do_py('--pyscript {}'.format(utils.quote_string_if_needed(args.script_path)))
 
         except KeyboardInterrupt:
             pass
@@ -3783,6 +3798,8 @@ def do_run_script(self, args: argparse.Namespace) -> Optional[bool]:
             self.perror("'{}' is not an ASCII or UTF-8 encoded text file".format(expanded_path))
             return
 
+        # Add some protection against accidentally running a Python file. The happens when users
+        # mix up run_script and run_pyscript.
         if expanded_path.endswith('.py'):
             self.pwarning("'{}' appears to be a Python file".format(expanded_path))
             selection = self.select('Yes No', 'Continue to try to run it as a text script? ')
diff --git a/tests/pyscript/raises_exception.py b/tests/pyscript/raises_exception.py
diff --git a/tests/pyscript/recursive.py b/tests/pyscript/recursive.py
@@ -0,0 +1,11 @@
+#!/usr/bin/env python
+# coding=utf-8
+# flake8: noqa F821
+"""
+Example demonstrating that calling run_pyscript recursively inside another Python script isn't allowed
+"""
+import os
+
+app.cmd_echo = True
+my_dir = (os.path.dirname(os.path.realpath(sys.argv[0])))
+app('run_pyscript {}'.format(os.path.join(my_dir, 'stop.py')))
diff --git a/tests/pyscript/run.py b/tests/pyscript/run.py
@@ -0,0 +1,6 @@
+# flake8: noqa F821
+import os
+
+app.cmd_echo = True
+my_dir = (os.path.dirname(os.path.realpath(sys.argv[0])))
+run(os.path.join(my_dir, 'to_run.py'))
diff --git a/tests/pyscript/to_run.py b/tests/pyscript/to_run.py
@@ -0,0 +1,2 @@
+# flake8: noqa F821
+print("I have been run")
diff --git a/tests/scripts/recursive.py b/tests/scripts/recursive.py
diff --git a/tests/test_cmd2.py b/tests/test_cmd2.py
@@ -236,6 +236,7 @@ def test_base_shell(base_app, monkeypatch):
     assert out == []
     assert m.called
 
+
 def test_base_py(base_app):
     # Create a variable and make sure we can see it
     out, err = run_cmd(base_app, 'py qqq=3')
@@ -263,17 +264,6 @@ def test_base_py(base_app):
     assert "NameError: name 'self' is not defined" in err
 
 
-@pytest.mark.skipif(sys.platform == 'win32',
-                    reason="Unit test doesn't work on win32, but feature does")
-def test_py_run_script(base_app, request):
-    test_dir = os.path.dirname(request.module.__file__)
-    python_script = os.path.join(test_dir, 'script.py')
-    expected = 'This is a python script running ...'
-
-    out, err = run_cmd(base_app, "py run('{}')".format(python_script))
-    assert expected in out
-
-
 def test_base_error(base_app):
     out, err = run_cmd(base_app, 'meow')
     assert "is not a recognized command" in err[0]
diff --git a/tests/test_run_pyscript.py b/tests/test_run_pyscript.py
@@ -32,7 +32,7 @@ def test_run_pyscript(base_app, request):
 
 def test_run_pyscript_recursive_not_allowed(base_app, request):
     test_dir = os.path.dirname(request.module.__file__)
-    python_script = os.path.join(test_dir, 'scripts', 'recursive.py')
+    python_script = os.path.join(test_dir, 'pyscript', 'recursive.py')
     expected = 'Recursively entering interactive Python consoles is not allowed.'
 
     out, err = run_cmd(base_app, "run_pyscript {}".format(python_script))
@@ -41,7 +41,7 @@ def test_run_pyscript_recursive_not_allowed(base_app, request):
 def test_run_pyscript_with_nonexist_file(base_app):
     python_script = 'does_not_exist.py'
     out, err = run_cmd(base_app, "run_pyscript {}".format(python_script))
-    assert "Error opening script file" in err[0]
+    assert "Error reading script file" in err[0]
 
 def test_run_pyscript_with_non_python_file(base_app, request):
     m = mock.MagicMock(name='input', return_value='2')
@@ -54,7 +54,7 @@ def test_run_pyscript_with_non_python_file(base_app, request):
 
 def test_run_pyscript_with_exception(base_app, request):
     test_dir = os.path.dirname(request.module.__file__)
-    python_script = os.path.join(test_dir, 'scripts', 'raises_exception.py')
+    python_script = os.path.join(test_dir, 'pyscript', 'raises_exception.py')
     out, err = run_cmd(base_app, "run_pyscript {}".format(python_script))
     assert err[0].startswith('Traceback')
     assert "TypeError: unsupported operand type(s) for +: 'int' and 'str'" in err[-1]
@@ -91,7 +91,7 @@ def test_run_pyscript_stop(base_app, request):
     # Verify onecmd_plus_hooks() returns True if any commands in a pyscript return True for stop
     test_dir = os.path.dirname(request.module.__file__)
 
-    # help.py doesn't run any commands that returns True for stop
+    # help.py doesn't run any commands that return True for stop
     python_script = os.path.join(test_dir, 'pyscript', 'help.py')
     stop = base_app.onecmd_plus_hooks('run_pyscript {}'.format(python_script))
     assert not stop
@@ -100,3 +100,11 @@ def test_run_pyscript_stop(base_app, request):
     python_script = os.path.join(test_dir, 'pyscript', 'stop.py')
     stop = base_app.onecmd_plus_hooks('run_pyscript {}'.format(python_script))
     assert stop
+
+def test_run_pyscript_run(base_app, request):
+    test_dir = os.path.dirname(request.module.__file__)
+    python_script = os.path.join(test_dir, 'pyscript', 'run.py')
+    expected = 'I have been run'
+
+    out, err = run_cmd(base_app, "run_pyscript {}".format(python_script))
+    assert expected in out

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+# flake8: noqa F821`
	`2`	`+print("I have been run")`