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

[wip][feat] completion cache, but very much just a PoC right now #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions okcli/completion_cache.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import hashlib
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have some test-coverage for these, please? Nothing too involved - just a few simple unit tests.

from os import path

def completion_filename(user, host):
return hashlib.md5(("%s%s" % (user, host)).encode('utf-8')).hexdigest()

def completion_cache_dir(config):
return path.join(path.dirname(config.filename), '.okcli_cache')
Copy link
Collaborator

Choose a reason for hiding this comment

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

os.path.dirname will only work if config.filename a full path - is it? (I'd have to check).


def completion_cache_file(user, host, config):
return path.join(completion_cache_dir(config), completion_filename(user, host))

def existing_completion_cache_file(user, host, config):
maybe_file = completion_cache_file(user, host, config)
if path.isfile(maybe_file):
return maybe_file
else:
return None
22 changes: 20 additions & 2 deletions okcli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
from .packages.special.main import NO_QUERY
from .sqlcompleter import SQLCompleter
from .sqlexecute import SQLExecute
from .completion_cache import (existing_completion_cache_file,
completion_cache_file,
completion_cache_dir)

click.disable_unicode_literals_warning = True
try:
Expand Down Expand Up @@ -106,6 +109,7 @@ def __init__(self, sqlexecute=None, prompt=None,
self.formatter = TabularOutputFormatter(
format_name=c['main']['table_format'])
self.syntax_style = c['main']['syntax_style']
self.cache_completions = c['main'].as_bool('cache_completions')
self.cli_style = c['colors']
self.wider_completion_menu = c['main'].as_bool('wider_completion_menu')
c_ddl_warning = c['main'].as_bool('ddl_warning')
Expand Down Expand Up @@ -640,17 +644,31 @@ def refresh_completions(self, reset=False):
if reset:
with self._completer_lock:
self.completer.reset_completions()
status_message = 'Auto-completion refresh started in the background.'
if self.cache_completions:
maybe_cache = existing_completion_cache_file(self.sqlexecute.user, self.sqlexecute.host, self.config)
Copy link
Collaborator

@cboddy cboddy Sep 23, 2020

Choose a reason for hiding this comment

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

Can we encapsulate everything in this if-branch to a method and wrap in a try-block to handle the scenario where something has gone wrong with the I/O?

if maybe_cache is not None:
import pickle
status_message = "%s (using cache for now)" % status_message
with open(maybe_cache, 'rb') as compfile:
new_completer = pickle.load(compfile)
self._swap_completer_objects(new_completer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if we don't use pickle for this if possible since:

  • pickle is a minor security concern
  • IIRC it's just a dict with strings so you could use json.load and json.dump.

self.completion_refresher.refresh(
self.sqlexecute, self._on_completions_refreshed,
{'smart_completion': self.smart_completion,
'supported_formats': self.formatter.supported_formats})

return [(None, None, None,
'Auto-completion refresh started in the background.')]
return [(None, None, None, status_message)]

def _on_completions_refreshed(self, new_completer):
self._swap_completer_objects(new_completer)

if self.cache_completions:
import pickle
Copy link
Collaborator

Choose a reason for hiding this comment

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

And the same here encapsulating this in a method and a try-block to handle any error please.

os.makedirs(completion_cache_dir(self.config), exist_ok=True)
with open(completion_cache_file(self.sqlexecute.user, self.sqlexecute.host, self.config), 'wb') as compfile:
pickle.dump(new_completer, compfile)

if self.cli:
# After refreshing, redraw the CLI to clear the statusbar
# "Refreshing completions..." indicator
Expand Down
5 changes: 5 additions & 0 deletions okcli/okclirc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
# possible completions will be listed.
smart_completion = True

# Caches completions until they have been refreshed.
# This makes sense when loading completions is not instantaneous
# (depends on the amount of things in the database)
cache_completions = False

# Multi-line mode allows breaking up the sql statements into multiple lines. If
# this is set to True, then the end of the statements must have a semi-colon.
# If this is set to False then sql statements can't be split into multiple
Expand Down