Skip to content

do_history no longer calls do_edit #795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tleonhardt merged 2 commits into from
Oct 31, 2019
Merged

do_history no longer calls do_edit #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
244af20
do_history no longer calls do_edit since edit is a command that is co…
kmvanbrunt Oct 30, 2019
1aa8d45
Merge branch 'master' into history_edit
tleonhardt Oct 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 16 additions & 13 deletions cmd2/cmd2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ def __init__(self, completekey: str = 'tab', stdin=None, stdout=None, *,
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()
Expand Down Expand Up @@ -3520,14 +3520,9 @@ def do_history(self, args: argparse.Namespace) -> Optional[bool]:
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)
Copy link
Member

@tleonhardt tleonhardt Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly calling the edit command see within the history command is a good thing as it removes the possibility of a pathological infinite loop which then allows us to put edit commands in the history.

# noinspection PyTypeChecker
self.do_run_script(quoted_fname)
self.do_run_script(utils.quote_string(fname))
finally:
os.remove(fname)
elif args.output_file:
Expand Down Expand Up @@ -3719,25 +3714,33 @@ def _generate_transcript(self, history: List[Union[HistoryItem, str]], transcrip
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"
" set editor (program-name)")

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)
Expand Down
17 changes: 3 additions & 14 deletions tests/test_history.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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')

Expand Down