Repository: python-cmd2/cmd2

Issue:
History should not call do_edit
The `do_history` method calls `do_edit` and this causes problems when a **cmd2** app has overridden the **edit** command. Change `do_history` to call non-command function for its edit functionality.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e8225782..45b726eb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,8 @@
 * Enhancements
     * Send all startup script paths to run_script. Previously we didn't do this if the file was empty, but that
     showed no record of the run_script command in history. 
+    * Made it easier for developers to override `edit` command by having `do_history` no longer call `do_edit`. This
+    also removes the need to exclude `edit` command from history list.
 
 ## 0.9.19 (October 14, 2019)
 * Bug Fixes
diff --git a/cmd2/cmd2.py b/cmd2/cmd2.py
index 6a600107..1af5e932 100644
--- a/cmd2/cmd2.py
+++ b/cmd2/cmd2.py
@@ -433,7 +433,7 @@ class Cmd(cmd.Cmd):
         self._initialize_history(persistent_history_file)
 
         # Commands to exclude from the history command
-        self.exclude_from_history = '''history edit eof'''.split()
+        self.exclude_from_history = ['eof', 'history']
 
         # Dictionary of macro names and their values
         self.macros = dict()
@@ -3520,14 +3520,9 @@ class Cmd(cmd.Cmd):
                     else:
                         fobj.write('{}\n'.format(command.raw))
             try:
-                # Handle potential edge case where the temp file needs to be quoted on the command line
-                quoted_fname = utils.quote_string(fname)
-
-                # noinspection PyTypeChecker
-                self.do_edit(quoted_fname)
-
+                self._run_editor(fname)
                 # noinspection PyTypeChecker
-                self.do_run_script(quoted_fname)
+                self.do_run_script(utils.quote_string(fname))
             finally:
                 os.remove(fname)
         elif args.output_file:
@@ -3719,7 +3714,7 @@ class Cmd(cmd.Cmd):
             msg = '{} {} saved to transcript file {!r}'
             self.pfeedback(msg.format(commands_run, plural, transcript_file))
 
-    edit_description = ("Edit a file in a text editor\n"
+    edit_description = ("Run a text editor and optionally open a file with it\n"
                         "\n"
                         "The editor used is determined by a settable parameter. To set it:\n"
                         "\n"
@@ -3727,17 +3722,25 @@ class Cmd(cmd.Cmd):
 
     edit_parser = Cmd2ArgumentParser(description=edit_description)
     edit_parser.add_argument('file_path', nargs=argparse.OPTIONAL,
-                             help="path to a file to open in editor", completer_method=path_complete)
+                             help="optional path to a file to open in editor", completer_method=path_complete)
 
     @with_argparser(edit_parser)
     def do_edit(self, args: argparse.Namespace) -> None:
-        """Edit a file in a text editor"""
+        """Run a text editor and optionally open a file with it"""
+        self._run_editor(args.file_path)
+
+    def _run_editor(self, file_path: Optional[str]) -> None:
+        """
+        Run a text editor and optionally open a file with it
+        :param file_path: optional path of the file to edit
+        :raises EnvironmentError if self.editor is not set
+        """
         if not self.editor:
             raise EnvironmentError("Please use 'set editor' to specify your text editing program of choice.")
 
         command = utils.quote_string(os.path.expanduser(self.editor))
-        if args.file_path:
-            command += " " + utils.quote_string(os.path.expanduser(args.file_path))
+        if file_path:
+            command += " " + utils.quote_string(os.path.expanduser(file_path))
 
         # noinspection PyTypeChecker
         self.do_shell(command)


Test patch:
diff --git a/tests/test_history.py b/tests/test_history.py
index 476cdd7e..4b900030 100755
--- a/tests/test_history.py
+++ b/tests/test_history.py
@@ -476,9 +476,9 @@ def test_history_edit(base_app, monkeypatch):
     # going to call it due to the mock
     base_app.editor = 'fooedit'
 
-    # Mock out the edit call so we don't actually open an editor
-    edit_mock = mock.MagicMock(name='do_edit')
-    monkeypatch.setattr("cmd2.Cmd.do_edit", edit_mock)
+    # Mock out the _run_editor call so we don't actually open an editor
+    edit_mock = mock.MagicMock(name='_run_editor')
+    monkeypatch.setattr("cmd2.Cmd._run_editor", edit_mock)
 
     # Mock out the run_script call since the mocked edit won't produce a file
     run_script_mock = mock.MagicMock(name='do_run_script')
@@ -590,17 +590,6 @@ def test_base_help_history(base_app):
     assert out == normalize(HELP_HISTORY)
 
 def test_exclude_from_history(base_app, monkeypatch):
-    # Set a fake editor just to make sure we have one.  We aren't
-    # really going to call it due to the mock
-    base_app.editor = 'fooedit'
-
-    # Mock out the subprocess.Popen call so we don't actually open an editor
-    m = mock.MagicMock(name='Popen')
-    monkeypatch.setattr("subprocess.Popen", m)
-
-    # Run edit command
-    run_cmd(base_app, 'edit')
-
     # Run history command
     run_cmd(base_app, 'history')