-
Notifications
You must be signed in to change notification settings - Fork 116
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.
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
Allow scripts to bypass adding their commands to the history #1291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem reasonable to me.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1291 +/- ##
==========================================
- Coverage 98.56% 93.37% -5.20%
==========================================
Files 22 21 -1
Lines 5776 5718 -58
==========================================
- Hits 5693 5339 -354
- Misses 83 379 +296 ☔ View full report in Codecov by Sentry. |
cmd2/cmd2.py
Outdated
@@ -5045,6 +5049,11 @@ def _current_script_dir(self) -> Optional[str]: | |||
help='record the output of the script as a transcript file', | |||
completer=path_complete, | |||
) | |||
run_script_parser.add_argument( | |||
'--history', | |||
action=argparse.BooleanOptionalAction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BooleanOptionalAction
was added in Python 3.9. Currently, cmd2 supports 3.7 and higher.
Consider changing this option name to --no-history
and changing action to store_true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laf0rge Would it be possible for you to make this change so it can work with Python 3.8?
@tleonhardt @anselor |
@kmvanbrunt That is a philosophical question. I can see advantages and disadvantages to either approach. Whatever works best for the majority of users is probably the right answer. My gut instinct is we shouldn't. But it depends on whether we want it to be more of a command history or an audit trail? Perhaps doing whatever bash command history would do in a similar situation would be the safest approach regarding customer expectations for behavior. |
I don't have a strong opinion on what the default should be, but I love having the setting so people can choose |
7a037df
to
493f271
Compare
@tleonhardt @kotfu @anselor @laf0rge Since the original author of this PR wasn't fond of his settable name ( I also removed his EDIT: I changed the name to |
493f271
to
ab1e83f
Compare
… scripts and pyscripts add commands to history.
ab1e83f
to
d6780a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach taken
At least in most of my use cases, the individual commands of a script executed by the command line should not end up in the history. If I go back in history, I want to find the commands I typed (like the
run_script
), and not the commands contained in the script I executed, or even the scripts that this script might execute itself. Imagine this happening on your bash/tcsh/zsh: Every time you execute a shell script your history is cluttered.I took care to preserve the existing behavior by default in these patches. However, if the cmd2 maintainers agree, it would of course also make sense (from my point of view) to simply never add the commands form scripts to the history and avoid all the configurability this patch introuces for backwards compatibility. Or introduce this patch, and change the default to "don't add to history".
Also, the naming of the settable and the run_script argument could possibly be improved, I just didn't come up with somtehing better so far.