Handle limited capacity history and the order, truncate history file as needed #8
Conversation
ldemailly
commented
Aug 8, 2024
•
ldemailly
commented
- Handle limited capacity history and the order, truncate history file as needed
- Fixes quote the commands in history (for when \n will work) #6
- Fixes dedup in history: don't add the same entry twice #7 because of Don't add adjacent duplicates to history term#3 included in term bump
- Use new ReplaceLatest to demonstrate keeping only valid commands (plus the last one so it can be edited)
…ng newest when loading/saving
…(plus the last one so it can be edited)
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
ldemailly
changed the title
| @@ -3,7 +3,7 @@ all: lint test demo | |||
| GO_BUILD_TAGS:=no_net,no_json,no_pprof | |||
|
|
|||
| demo: | |||
| go run -tags $(GO_BUILD_TAGS) ./example/ -loglevel debug | |||
| go run -tags $(GO_BUILD_TAGS) ./example/ -loglevel debug -only-valid | |||
There was a problem hiding this comment.
| go run -tags $(GO_BUILD_TAGS) ./example/ -loglevel debug -only-valid | |
| go run -tags $(GO_BUILD_TAGS) ./example/ -loglevel debug -only-valid |
I'm a crazy person 🤪
BTW, do you know unmake?
Makefile linter
https://github.com/mcandre/unmake
There was a problem hiding this comment.
I can tell :) fixing it, installed unmake but will ignore
warning: Makefile: MAKEFILE_PRECEDENCE: lowercase Makefile to makefile for launch speed
warning: Makefile:1: STRICT_POSIX: lead makefiles with the ".POSIX:" compliance marker, or else rename include files like *.include.mk
warning: Makefile:8: COMMAND_COMMENT: comment embedded inside commands will forward to the shell interpreter
warning: Makefile:14: COMMAND_COMMENT: comment embedded inside commands will forward to the shell interpreter
and
error: Makefile:3:15 found "=", expected: LF, comment, inline command, macro expansion, target, wait prerequisite marker
(it doesn't know about := ? uh?)
There was a problem hiding this comment.
Dunno about unmake, but it helped me to spot some issues
| func AddOrReplaceHistory(t *terminal.Terminal, replace bool, l string) { | ||
| // if in default auto mode, we don't manage history | ||
| // we also don't add empty commands. (at start of program) | ||
| if t.AutoHistory() || l == "" { |
There was a problem hiding this comment.
You could also add something to do not log command starting with space, as it's a common pattern to ignore thing in history
There was a problem hiding this comment.
never heard of leading space preventing history before:
$ echo "this had space"
$ history | tail
568 echo "this had space"
569 history | tail
also I do want spaces in history for grol indentation
There was a problem hiding this comment.
The line is removed when stored, not in local history
| t.autoHistory = enabled | ||
| t.term.AutoHistory(enabled) | ||
| } | ||
|
|
||
| // AutoHistory returns the current auto history setting. | ||
| func (t *Terminal) AutoHistory() bool { | ||
| return t.autoHistory |
There was a problem hiding this comment.
I know you forked golang/term in fortio, but I find strange that you now have two flags about autoHistory, one in fortio/term and one in fortio/terminal
BTW, maybe renaming fortio/term to something like fortio/golang-term-fork would make it easier to understand why fortio has now two "term" packages
There was a problem hiding this comment.
as mentioned, this one (terminal) is standalone and fully encapsulate what happens to be an implementation detail
this being said, others that for instance don't want fortio cli/logger/etc... might use the lower level term directly if they also don't want to wait for PRs to merge upstream (which may or may not ever happen)
