-
Notifications
You must be signed in to change notification settings - Fork 2.3k
DRAFT: Update themes to use new Automatic History Management #1951
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
Changes from all commits
d9b29b3
0758b86
a8f2dc5
8a3cc89
24a4067
7823e36
569b8b8
db04b2b
ee65fd8
26b2a8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,18 @@ | ||
# shellcheck shell=bash | ||
# shellcheck disable=SC2034,SC2154 | ||
|
||
case $HISTCONTROL in | ||
*'auto'*) | ||
: # Do nothing, already configured. | ||
;; | ||
*) | ||
# Append new history lines to history file | ||
HISTCONTROL="${HISTCONTROL:-}${HISTCONTROL:+:}autosave" | ||
;; | ||
esac | ||
safe_append_preexec '_bash-it-history-auto-load' | ||
safe_append_prompt_command '_bash-it-history-auto-save' | ||
Comment on lines
+4
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, there are too many repetitions of this code section. These lines should be factorized into an independent function (defined in e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed. |
||
|
||
SCM_THEME_PROMPT_DIRTY='' | ||
SCM_THEME_PROMPT_CLEAN='' | ||
SCM_GIT_CHAR="${bold_cyan}±${normal}" | ||
|
@@ -30,9 +42,7 @@ else | |
fi | ||
|
||
function prompt_setter() { | ||
# Save history | ||
_save-and-reload-history 1 | ||
PS1=" | ||
PS1=" | ||
$(clock_prompt) $(scm_char) [${THEME_PROMPT_HOST_COLOR}\u@${THEME_PROMPT_HOST}$reset_color] $(virtualenv_prompt)$(ruby_version_prompt)\w | ||
$(scm_prompt)$reset_color $ " | ||
PS2='> ' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,20 @@ function prompt_setter() { | |
scm_char="$(scm_char)" | ||
scm_prompt_info="$(scm_prompt_info)" | ||
ruby_version_prompt="$(ruby_version_prompt)" | ||
_save-and-reload-history 1 # Save history | ||
PS1="(${clock_prompt}) ${scm_char} [${blue?}\u${reset_color?}@${green?}\H${reset_color?}] ${yellow?}\w${reset_color?}${scm_prompt_info}${ruby_version_prompt} ${reset_color?} " | ||
PS2='> ' | ||
PS4='+ ' | ||
} | ||
|
||
case $HISTCONTROL in | ||
*'auto'*) | ||
: # Do nothing, already configured. | ||
;; | ||
*) | ||
# Append new history lines to history file | ||
HISTCONTROL="${HISTCONTROL:-}${HISTCONTROL:+:}autosave" | ||
;; | ||
esac | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hooks into preexec and prompt_command are missing. All the themes touched in this PR seems to have called edit: Anyway, I suggest removing all these additional hooks as discussed in #1951 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question is do I keep fixing this or just close and bury it? Maybe it's just the wrong approach altogether. |
||
safe_append_prompt_command prompt_setter | ||
|
||
SCM_THEME_PROMPT_DIRTY=" ✗" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to
_bash-it-history-init
(inlib/history.bash
), the commands forpreexec
andprompt_command
are reversed.bash-it/lib/history.bash
Lines 5 to 8 in dc25be3
Thinking of the functions that
auto-load
andauto-save
perform, I think the "save" should be performed before running the command, and the "load" should be performed before the prompt command.In addition, why don't they simply call
_bash-it-history-init
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it seems
_bash-it-history-init
is anyway forcibly loaded at the end ofbash_it.sh
through_bash_it_library_finalize_hook
.bash-it/lib/history.bash
Line 49 in dc25be3
One possibility is that the above "reversed hooks" are intended to complement load/save that haven't been registered by
_bash-it-history-init
to perform both load/save in both preexec and prompt_command. However, I don't see the reason for running both twice before running the command and after running the command. The default hooks set up by_bash-it-history-init
seem sufficient to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to remove all
safe_append_preexec '_bash-it-history-auto-load'
andsafe_append_prompt_command '_bash-it-history-auto-save'
from this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in other words, it really needs to be redone. I'll dump it.