[wip][feat] completion cache, but very much just a PoC right now #7
Conversation
There was a problem hiding this comment.
This is almost ready as is IMHO. If you could take a look at the comments I'd like to try it locally before merging, then we can push a new release to pypi.
I've updated setup.py on master to be py-3 only since some of the dependencies have become so, you may need to rebase.
To run the tests can you run pytest test? This is what circle-ci does too, thanks,
| 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) |
There was a problem hiding this comment.
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
dictwith strings so you could usejson.loadandjson.dump.
| @@ -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) | |||
There was a problem hiding this comment.
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?
|
|
||
| def _on_completions_refreshed(self, new_completer): | ||
| self._swap_completer_objects(new_completer) | ||
|
|
||
| if self.cache_completions: | ||
| import pickle |
There was a problem hiding this comment.
And the same here encapsulating this in a method and a try-block to handle any error please.
| 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') |
There was a problem hiding this comment.
os.path.dirname will only work if config.filename a full path - is it? (I'd have to check).
| @@ -0,0 +1,18 @@ | |||
| import hashlib | |||
There was a problem hiding this comment.
Can we have some test-coverage for these, please? Nothing too involved - just a few simple unit tests.
|
Thanks for the review @cboddy ! Will look into making the suggested changes as soon as I have some breathing room! |
Hi,
As discussed #6 here's a draft at completion caching.
Definitely don't merge it for now.
Want to get some early feedback since I haven't written python in a while.
I've just implemented the config file switch for now, it might actually be good enough because IMHO there's no significant downside to activating the completion cache always. If fetching the completions is fast you won't notice and if it's not you want the cache 😛
The usage of pickle might be debatable, since it can be used to execute malicious code (some other thing could tamper with the cache, but then your machine is already compromised, etc). Pickle has the advantage that it just works on python platforms and it doesn't care about the structure of the completer.
I've also just tested this by setting the pythonpath and running it locally.
setup.py testtries to build a wheel and fails for some reason, and I haven't found instructions on how to run the existing tests, but I'm partial to adding some.Other stuff I'm not sure about: