-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Rebuild terlar prompt as a configuration of fish_git_prompt #7918
Conversation
(needs some more testing) |
tests/checks/git.fish
Outdated
@@ -46,6 +46,8 @@ echo | |||
#CHECK: (newbranch|…1) | |||
set -e __fish_git_prompt_show_informative_status | |||
|
|||
set -e __fish_git_prompt_show_informative_status |
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.
Huh deleting it twice won't trigger another handler, right? Unless there is a spooky universal variables around...
(Re the commit message: the variable handler behavior for variable overrides seems correct as far as I can tell)
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.
Oh this is a rebase artifact (I did this for #7926 as well).
(Re the commit message: the variable handler behavior for variable overrides seems correct as far as I can tell)
The problem is that the handler only triggers when the override goes into scope, not also when it goes out of scope. So since we disable informative mode stuff when the variable is erased, we don't notice when it's used as an override.
See:
> set -l foo bar
> function on-foo --on-variable foo; echo on foo: $argv; echo foo is $foo; end
> foo=banana echo bar
on foo: VARIABLE SET foo
foo is banana
bar
See how the handler is never triggered for foo being "bar" again? That's the problem here.
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.
I'm not entirely sure how this should act, just saying that it gets in the way here.
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.
That sounds correct, because the override acts just like begin; set -l foo banana; ...; end
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.
That's consistent, I'm not sure it's correct.
Maybe we should fire the variable handlers when it goes out of scope? Or would that massively slow down block closing?
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.
I guess this problem has just never come up before because variable handlers are primarily used on global/universal variables.
Firing the handlers seems useful here - for running the git prompt with different configurations via variable overrides.
For this use case it's enough to fire handlers when a variable, that shadows another variables, goes out of scope; not when a local-only variable dies.
(Ideally we don't need the caching logic of ___fish_git_prompt_init
, but it's probably a worthwhile optimization).
Sounds great! I think it's okay if there are minor differences. |
This is a neat feature of the terlar git prompt
And then only define it in informative mode.
This removes the awkward secondary logic. Note that we still ship a function called `__terlar_git_prompt` because people who picked the prompt will still be calling it - we don't update the prompt.
The terlar prompt uses `|branch`, this is visually quite important.
I'm not currently working on this - there's some annoying subtleties that I don't have the right frame of mind to work on atm, so I'm moving it out of the 3.3 milestone. |
Yeah, closing for now. If I ever get back to this I can just reopen it, no need to keep the draft around. |
Description
The
terlar
sample prompt uses its own git prompt implementation called__terlar_git_prompt
, which was always a bit cheesy and meant that it was harder to adjust than other prompts.This rebuilds that function as a wrapper around fish_git_prompt that just sets some variables.
To do so it adds two features to fish_git_prompt to achieve parity:
Note that this doesn't remove the
__terlar_git_prompt
function because that would break every configuration that currently uses the terlar prompt, which is quite awkward. If we wanted to remove it we would have to figure out how to tell users to switch. (which isn't too much to ask, all they'd have to do is copy the sample again!)Part of #7884.
TODOs: