Revise the vi mode indicator to ❮ for non-insert modes #140
Conversation
There was a problem hiding this comment.
Thanks for the PR 👍
I reviewed your code and requested some changes.
Feel free to ask for clarification.
Could you add automated tests in order to validate your feature before merge:
- verify the variable is correctly set in conf.d/pure.fish via _tests/pure.test.fish
- verify the behavior is working as expected when configuration is
true/false, see _tests/pure_prompt_vimode.test.fish
functions/_pure_prompt_vimode.fish
Outdated
| @@ -1,3 +1,6 @@ | |||
| function _pure_prompt_vimode | |||
| echo (fish_default_mode_prompt) | |||
| set --local reverse_symbol_instead_of_default_mode_prompt $pure_reverse_prompt_symbol_in_vimode | |||
There was a problem hiding this comment.
same here, no need for a proxy variable, use the original one directly, the code will be easier to read and maintain 👍
functions/_pure_prompt_symbol.fish
Outdated
| if test reverse_symbol_instead_of_default_mode_prompt | ||
| # Do nothing if not in vi mode | ||
| if test "$fish_key_bindings" = "fish_vi_key_bindings" | ||
| or test "$fish_key_bindings" = "fish_hybrid_key_bindings" |
There was a problem hiding this comment.
You should use string match and glob/regex to test the key_binding mode
if test (string match fish_{vi,hybrid}_key_bindings $fish_key_bindings)
or to be more explicit:
set --local is_vi_mode (string match fish_{vi,hybrid}_key_bindings $fish_key_bindings)
if test is_vi_mode
|
Thanks for the comments. I revised the PR to address your comments on the code, and took all your suggestions. With regards to testing, I added one simple test for the config variable. However, both on my local machine and the CI checks, the tests are not providing meaningful feedback—I am getting Given that I'm not able to see if the test is passing or not, I'm not particularly comfortable with writing a more complex test (especially since I'm not familiar with fish scripting or with whatever this test environment is). If anyone else is more familiar with testing/has a setup under which the tests run, they should feel free to add an additional test for the rest of the functionality. |
There was a problem hiding this comment.
Thanks, that's look good.
Kudos for trying to implement tests! 🙏
I'm marking as requesting changes as I did the review on my phone and will work on it to add the tests.
The error you described in the test might be due to fishtape, that had a new major release, and the way we install it.
This commit changes the default behavior of the prompt in vi mode. Previously, the prompt retained the fish shell's default mode indicator: it would print `[I]`, `[N]`, or `[V]` to indicate whether the user was in insert, normal, or visual mode. After this commit, the prompt will flip to `❮` whenever the user is not in insert mode. This matches the behavior of the zsh version of Pure. The user can restore the prior functionality by setting the `pure_reverse_prompt_symbol_in_vimode` variable to `false` in their conf.d/pure.fish file.
edouard-lopez
force-pushed
the
vi_mode
branch
from
6be4717
to
ef543ef
Compare
|
@codesections I committed some change, could you test it's the expected behaviour? |
|
Yes, it seems to work great. Thanks! |
related: #139
This PR changes the default behavior of the prompt in vi mode, as discussed in #139.
Previously, the prompt retained the fish shell's default mode indicator: it would print
[I],[N], or[V]to indicate whether the user was in insert, normal, or visual mode.After this commit, the prompt will flip to
❮whenever the user is not in insert mode. This matches the behavior of the zsh version of Pure.The user can restore the prior functionality by setting the
pure_reverse_prompt_symbol_in_vimodevariable tofalsein their conf.d/pure.fish file.