Persistent history for #669 #689
Conversation
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
==========================================
+ Coverage 95.29% 95.46% +0.16%
==========================================
Files 11 11
Lines 3191 3241 +50
==========================================
+ Hits 3041 3094 +53
+ Misses 150 147 -3
Continue to review full report at Codecov.
|
|
Changes look good. I haven't had a chance to manually test yet, but plan to do so soon. If we could get test coverage up of new code that would be a good thing. |
python 3.4 pathlib doesn’t have .expanduser()
…t HistoryItem unit test
There was a problem hiding this comment.
I did some manual testing and everything works well and as expected. I think we have an important missing feature and some minor documentation updates that need to be done. My view of remaining work is in the TODO below.
TODO:
- Add a
-a/--allflag to the history command and make the history command only show history from the current session by default, but the complete history when the-aflag is given
- An alternative would be to add a boolean parameter to
cmd2.Cmd.__init__which determines whether or not the cmd2 history gets populated based on thepersistent_history_file(but readline history would always be populated)
- Minor doc update of
cmd2.Cmd.__init__()docstring for persistent history parameters - Minor doc update of Searchable Command History section of Sphinx docs
I found that at least with certain versions of Python and OSes, if I had a previous text-based readline history, an unhandled UnpicklingError exception could occur. So now we catch that and several other possible errors which can theoretically occur during unpickling and just move on with an empty history. The old file will get overwritten with one in the new format when the application exits.
… including those persisted from previous sessions
Also:
- History class has been modified to keep track of the session start index
- History class span(), str_search(), and regex_search() methods now take an optional 2nd boolean parameter `include_persisted` which determines whether or not commands persisted from previous sessions should be included by default
- If a start index is manually specified, then it automatically includes the full search
- Updates unit tests
|
@kotfu and/or @kmvanbrunt The I also added some more exceptions to catch when attempting to unpickle a persistent history file to deal with some errors I found when a pre-existing readline text file existed. |
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
==========================================
+ Coverage 95.29% 95.46% +0.16%
==========================================
Files 11 11
Lines 3191 3241 +50
==========================================
+ Hits 3041 3094 +53
+ Misses 150 147 -3
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
==========================================
+ Coverage 95.29% 95.46% +0.16%
==========================================
Files 11 11
Lines 3191 3241 +50
==========================================
+ Hits 3041 3094 +53
+ Misses 150 147 -3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #689 +/- ##
==========================================
+ Coverage 95.29% 95.46% +0.16%
==========================================
Files 11 11
Lines 3191 3241 +50
==========================================
+ Hits 3041 3094 +53
+ Misses 150 147 -3
Continue to review full report at Codecov.
|
|
|
||
| # cmd2 history file used in hello_cmd2.py | ||
| cmd2_history.txt | ||
| cmd2_history.dat |
There was a problem hiding this comment.
Maybe ignore both the old and new name just to be safe?
…t no longer is dependent on readline. Made certain paths absolute since the CWD could change between usages.
| # Clear command and readline history | ||
| self.history.clear() | ||
|
|
||
| if self.persistent_history_file: |
|
May I inquire why the change to binary fromat for something that is supposed to be human readable ? It's great to be able to just open an editor and look at the history or even to prefill an history. |
|
why do we need 2 histories ? |
|
@teto The change to the format of the persistent history file is because we had a user request, #669, to persist the For this, the core In regards to why we want two histories, there are several reasons. First off If your |
|
I don't really mind the 2 histories. as a user, it mostly confuses me but that's fine. Hadn't thought of windows users I admit :/ I was just a bit surprised by the
if people don't care what's the motivation for changing (btw I do care :p ) ? Any advantage of binary serialisation doesn't make sense for this usecase. Neither bash or zsh serialise the history. I like to have a look at it. What if I want to sync it on git ? I am not familiar with pickling but I fear it's not portable. What if the computer unexpectedly shuts down ? with zsh I can just open the history file and remove the last line to fix it but I don't think that will be possible with pickling. |
|
Looking at the changelog, seems like the history items has grown to some big object, which would explain why one wouId want to use pickle. I will regret the txt based format but not enough to nag the nice maintainers :p thanks for your work. |
|
A couple of thoughts about concerns raised by @teto.
|
Fixes #669.
We have a readline history and a native in-memory history. Previous to this PR, only readline history was persistent between invocations of a
cmd2based app. Now both readline and native history are persistent. Plaintext history files are ignored, and do not cause read errors. All new history files written after this PR sill be in binary pickle format.This includes several breaking changes noted in the changelog, including: