History clear #467
History clear #467
Conversation
Added better error checking when loading readline history file Improved some error messages Changed IOError usages to OSError since they were merged in Python 3.3.
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
==========================================
- Coverage 90.2% 89.84% -0.36%
==========================================
Files 10 10
Lines 2736 2758 +22
==========================================
+ Hits 2468 2478 +10
- Misses 268 280 +12
Continue to review full report at Codecov.
|
| # than trying to open the file with write access since readline's underlying function needs to | ||
| # create a temporary file in the same directory and may not have permission. | ||
| readline.set_history_length(persistent_history_length) | ||
| readline.write_history_file(persistent_history_file) |
There was a problem hiding this comment.
Thanks for adding better error checking to make sure the user can actually write to the persistent history file.
| readline.read_history_file(persistent_history_file) | ||
| except FileNotFoundError: | ||
| pass | ||
| except OSError as ex: |
There was a problem hiding this comment.
This is a good change since various exceptions got merged into OSError in Python 3.3 and we no longer support earlier versions of Python
| readline.set_history_length(persistent_history_length) | ||
| readline.write_history_file(persistent_history_file) | ||
| except OSError as ex: | ||
| self.perror("readline cannot write persistent history file '{}': {}". |
There was a problem hiding this comment.
Yeah, we should have had an error message in this case. Thanks for adding it.
| history_parser_group.add_argument('-s', '--script', action='store_true', help='script format; no separation lines') | ||
| history_parser_group.add_argument('-o', '--output-file', metavar='FILE', help='output commands to a script file') | ||
| history_parser_group.add_argument('-t', '--transcript', help='output commands and results to a transcript file') | ||
| history_parser_group.add_argument('-c', '--clear', action="store_true", help='clears all history') |
| # Make sure expanded_path points to a file | ||
| if not os.path.isfile(expanded_path): | ||
| self.perror('{} does not exist or is not a file'.format(expanded_path), traceback_war=False) | ||
| self.perror("'{}' is not a file".format(expanded_path), traceback_war=False) |
There was a problem hiding this comment.
Could alternatively use {!r}, but this is fine
| output = run_cmd(base_app, 'history -r 1') | ||
| assert output == expected | ||
|
|
||
| def test_history_clear(base_app): |
There was a problem hiding this comment.
Thanks for adding unit tests
|
@kmvanbrunt Better code coverage of new/added code would be nice. But this PR looks fine to me. |
Added --clear argument to history command
Added better error checking when loading readline history file
Improved some error messages
Changed IOError usages to OSError since they were merged in Python 3.3.