-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fixes/changes to interactive history #5636
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5636 |
0.3.17 | ||
++++++ | ||
* Persist history across different sessions | ||
* History while in non-default scope disappears after scope is changed and is not saved. |
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 don't really understand this. If I was in a scope, say "network" and ran public-ip create ...
why would network public-ip- create ...
not appear in history? It sounds like public-ip create
would appear while I'm in the "network" scope but then when I change scope, that history vanishes. Is that right?
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.
Yes, that's right, the reason why I did this is because the scoping commands %% <whatever>
would not apply once the scope is changed, so at the very least, they should not be saved.
The other option would be to match for different scenarios and save accordingly. i.e., scope commands are not saved, cli commands have the scope appended, non-azure cli commands (using #
) are saved alone, etc.
In addition, inside a scope, some history is no longer valid. i.e. all commands from other scopes. Thus, we would also need to parse through the history if all scopes use the same source.
This can be confusing, a user may wonder why some things are showing up, some aren't, and why some differ from what they typed.
To me, having a simpler rule defined, where scoping creates a different "environment" of sorts with its own history seems much more intuitive. Once that environment is left, the history is cleaned up and gone.
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.
After discussion with @tjprescott, decided to go with one source of history for all commands, no matter the scope.
It will be up to the user to deal with history from different scopes.
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.
After our discussion, I think what we agreed upon was:
- not deleting the history regardless of scope
- just presenting the text history, regardless of scope
IMO its a little less user-focused, but it's better than being broken. If you add support for ~ then bonus points :)
b_flag, c_flag, outside, cmd = self._special_cases(cmd, outside) | ||
if not self.default_command: |
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.
What is default_command?
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.
It's the scope; empty string if not in one.
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.
Seems to me then we should still not commit the default command to history? We wouldn't want empty strings appended.
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.
We don't. The default_command is no longer included within history.
We append "text" to history as long as it is not empty.
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.
LGTM
Closes: #5625
Closes: #5633
Closes: #3016
-Persist history across different interactive sessions
-some scoping logic bug fixes, enabled history inside scopes
-fixed
clear-history
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines