Skip to content
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

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

laf0rge
Copy link
Contributor

@laf0rge laf0rge commented Jan 31, 2024

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.

anselor
anselor previously approved these changes Feb 16, 2024
Copy link
Contributor

@anselor anselor left a 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.

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.37%. Comparing base (2bc8b6b) to head (7a037df).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
cmd2/cmd2.py 90.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (2bc8b6b) and HEAD (7a037df). Click for more details.

HEAD has 10 uploads less than BASE
Flag BASE (2bc8b6b) HEAD (7a037df)
15 5
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.
📢 Have feedback on the report? Share it here.

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,
Copy link
Member

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.

Copy link
Member

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?

@kmvanbrunt
Copy link
Member

@tleonhardt @anselor
What about commands run in a pyscript via run_pyscript?
Should we just go with the approach to not add any commands run in scripts or pyscripts to the history?

@tleonhardt
Copy link
Member

@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.

@kotfu
Copy link
Member

kotfu commented Sep 13, 2024

I don't have a strong opinion on what the default should be, but I love having the setting so people can choose

@kmvanbrunt
Copy link
Member

kmvanbrunt commented Sep 19, 2024

@tleonhardt @kotfu @anselor @laf0rge
I completed this PR by accounting for the new settable in run_pyscript, adding unit tests, and making documentation updates.

Since the original author of this PR wasn't fond of his settable name (history_includes_scripts), I changed it to save_scripted_commands. I also considered log_scripted_commands, but I didn't want to confuse those who use cmd2-based apps with a log file. I'm open to changing the settable name if you have suggestions.

I also removed his --history option to run_script which temporarily overrides self.save_scripted_commands. If you think it should be restored, let me know and I will also add it to run_pyscript.

EDIT: I changed the name to scripts_add_to_history.

… scripts and pyscripts

add commands to history.
Copy link
Member

@tleonhardt tleonhardt left a 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

@tleonhardt tleonhardt merged commit 522ce2e into python-cmd2:master Sep 21, 2024
44 of 59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants