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.

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

Fixes/changes to interactive history #5636

Merged
merged 3 commits into from
Feb 24, 2018
Merged

Fixes/changes to interactive history #5636

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0793c38
fixed history persistence, logic
williexu Feb 20, 2018
e1c7b24
version bump
williexu Feb 22, 2018
df5fdf2
single source for history, fix clear-history
williexu Feb 22, 2018
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
5 changes: 5 additions & 0 deletions src/command_modules/azure-cli-interactive/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
Release History
===============

0.3.17
++++++
* Persist history across different sessions
* History while in non-default scope disappears after scope is changed and is not saved.
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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:

  1. not deleting the history regardless of scope
  2. 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 :)


0.3.16
++++++
* Fix issue where user is prompted to login when using interactive mode in Cloud Shell.
Expand Down
29 changes: 19 additions & 10 deletions src/command_modules/azure-cli-interactive/azclishell/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
from azclishell.frequency_heuristic import DISPLAY_TIME, frequency_heuristic
from azclishell.gather_commands import add_new_lines, GatherCommands
from azclishell.key_bindings import InteractiveKeyBindings
from azclishell.layout import create_layout, create_tutorial_layout, set_scope
from azclishell.layout import create_layout, create_tutorial_layout, set_scope, get_scope
from azclishell.progress import progress_view
from azclishell.telemetry import Telemetry
from azclishell.threads import LoadCommandTableThread
Expand Down Expand Up @@ -88,8 +88,8 @@ def restart_completer(shell_ctx):
# pylint: disable=too-many-instance-attributes
class AzInteractiveShell(object):

def __init__(self, cli_ctx, style=None, completer=None, styles=None,
lexer=None, history=InMemoryHistory(),
def __init__(self, cli_ctx, style=None, completer=None,
lexer=None, history=None,
app=None, input_custom=sys.stdin, output_custom=None,
user_feedback=False, intermediate_sleep=.25, final_sleep=4):

Expand All @@ -106,7 +106,8 @@ def __init__(self, cli_ctx, style=None, completer=None, styles=None,
self.completer = completer or AzCompleter(self, GatherCommands(self.config))
except IOError: # if there is no cache
self.completer = None
self.history = history or FileHistory(os.path.join(self.config.config_dir, self.config.get_history()))
self._default_history = history or FileHistory(os.path.join(self.config.config_dir, self.config.get_history()))
self.history = self._default_history
os.environ[ENV_ADDITIONAL_USER_AGENT] = 'AZURECLISHELL/' + __version__
self.telemetry = Telemetry(self.cli_ctx)

Expand Down Expand Up @@ -142,7 +143,7 @@ def __init__(self, cli_ctx, style=None, completer=None, styles=None,
def __call__(self):

if self.cli_ctx.data["az_interactive_active"]:
logger.warning("You're in the interactive shell already.\n")
logger.warning("You're in the interactive shell already.")
return

if self.config.BOOLEAN_STATES[self.config.config.get('DEFAULT', 'firsttime')]:
Expand Down Expand Up @@ -527,7 +528,7 @@ def _special_cases(self, cmd, outside):
elif SELECT_SYMBOL['example'] in cmd:
cmd, continue_flag = self.handle_example(cmd, continue_flag)
self.telemetry.track_ssg('tutorial', cmd)
elif len(cmd_stripped) > 2 and SELECT_SYMBOL['scope'] == cmd_stripped[0:2]:
elif SELECT_SYMBOL['scope'] == cmd_stripped[0:2]:
continue_flag, cmd = self.handle_scoping_input(continue_flag, cmd, cmd_stripped)

return break_flag, continue_flag, outside, cmd
Expand Down Expand Up @@ -579,6 +580,7 @@ def handle_scoping_input(self, continue_flag, cmd, text):
cmd = cmd.replace(SELECT_SYMBOL['scope'], '')

continue_flag = True
original_scope = get_scope()

if not default_split:
self.default_command = ""
Expand All @@ -604,8 +606,7 @@ def handle_scoping_input(self, continue_flag, cmd, text):
cmd = cmd.replace(SELECT_SYMBOL['scope'], '')
self.telemetry.track_ssg('scope command', value)

elif SELECT_SYMBOL['unscope'] == default_split[0] and \
len(self.default_command.split()) > 0:
elif SELECT_SYMBOL['unscope'] == default_split[0] and self.default_command.split():

value = self.default_command.split()[-1]
self.default_command = ' ' + ' '.join(self.default_command.split()[:-1])
Expand All @@ -619,8 +620,16 @@ def handle_scoping_input(self, continue_flag, cmd, text):
print("Scope must be a valid command", file=self.output)

default_split = default_split[1:]
if not get_scope():
self.set_history(history=self._default_history)
elif get_scope() != original_scope:
self.set_history(history=InMemoryHistory())
return continue_flag, cmd

def set_history(self, history=None):
self.history = history
self.cli.buffers[DEFAULT_BUFFER].history = history

def cli_execute(self, cmd):
""" sends the command to the CLI to be executed """

Expand Down Expand Up @@ -709,9 +718,9 @@ def run(self):
# when the user pressed Control D
break
else:
self.history.append(text)
b_flag, c_flag, outside, cmd = self._special_cases(cmd, outside)
if not self.default_command:
Copy link
Member

Choose a reason for hiding this comment

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

What is default_command?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

self.history.append(text)

if b_flag:
break
if c_flag:
Expand Down
1 change: 0 additions & 1 deletion src/command_modules/azure-cli-interactive/azclishell/key_bindings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ def pan_down(event):
def config_settings(event):
""" opens the configuration """
shell_ctx.telemetry.track_key('F1')

shell_ctx.is_prompting = True
config = shell_ctx.config
answer = ""
Expand Down
2 changes: 1 addition & 1 deletion src/command_modules/azure-cli-interactive/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
cmdclass = {}

# Version is also defined in azclishell.__init__.py.
VERSION = "0.3.16"
VERSION = "0.3.17"
# The full list of classifiers is available at
# https://pypi.python.org/pypi?%3Aaction=list_classifiers
CLASSIFIERS = [
Expand Down