Skip to content

git prompt: Interpret values as true for bools instead of relying on defined-or-not #9274

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

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

faho
Copy link
Member

@faho faho commented Oct 13, 2022

This allows explicitly turning these settings off by setting the variable to e.g. 0.

See #7120

This is technically a change in behavior iff you set these variables to anything but "1". I don't think I've ever seen that, and it's easy enough to fix.

Note: This lacks the "hide" stuff (__fish_git_prompt_hide_stagedstate and friends), it's not documented, nigh-useless now that we properly look at the status order. I want to remove it.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

This allows explicitly turning these settings off by setting the variable to e.g. 0.

See fish-shell#7120
@faho faho added this to the fish 3.6.0 milestone Oct 13, 2022
@mqudsi
Copy link
Contributor

mqudsi commented Oct 14, 2022

I'm not sure how I feel about this. I agree that it should be possible to set them to some value to indicate that they are disabled, but to assume that in a loosely typed scripting language everyone will by default understand or muscle memory default to a numeric 1 for truthiness is not sitting well with me.

You've gone from accepting any value as "true" but not having a way to express "false" to understanding every variable to be "false" except for the one and only value reserved as "true" (and that value isn't a literal true, btw).

In writing GUI frontends for CLIs, I have found the greatest success in considering any of on, true, 1, or yes (all case-insensitive, of course) to be equivalent to true. I'm not suggesting we document that (by all means, go ahead and keep the documentation the same and insist that users use 1) but we should be more liberal in parsing even if that means we need a helper function like is_true <var_name>.

@faho
Copy link
Member Author

faho commented Oct 14, 2022

I agree that it should be possible to set them to some value to indicate that they are disabled, but to assume that in a loosely typed scripting language everyone will by default understand or muscle memory default to a numeric 1 for truthiness is not sitting well with me.

It's quite simply how people use this:

https://github.com/search?q=__fish_git_prompt_show_informative_status&type=code

By a country mile, "1" is used to signify true.

It's also how we do it, it's how we set $fish_private_mode, $fish_term24bit, $fish_use_posix_spawn, $fish_handle_reflow and other random helper variables.

Statistically, 1 has won. "true" is a distant second.

And I'm not sold that having multiple true values is helpful - like yes, we have a cheesy "bool_from_string" helper that is technically used for some of these, but people don't use that capability, and having one true value makes the check easier.

This is the absolute, 100% correct check to test if $fish_handle_reflow is a true value:

test "$fish_handle_reflow" = 1

This is easy to get right (well, ignoring tests issues). On the other hand, this is the check to see if $fish_term24bit is set to true:

string match -ri '^[yt1]' -- $fish_term24bit

So these values are true: "yakshaving", "Tarpit", "1WRONG" and these are not: "ja", "waar", "oui".

any of on, true, 1, or yes (all case-insensitive, of course)

Case-insensitive means you can't use contains or `test´ (unless you want to enumerate all the possibilities), so this is something like:

string match -qir '^(on|true|1|yes)$' -- $foo

And having one value makes not just checking it easier, it also makes checking for it, grepping for it, easier.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 14, 2022

GitHub's default code search includes too many duplicates. Using cs.github.com shows much more variety, including "yes" and "true" (and even "hi", blank values, and some unicode symbols!) - and I didn't even use a regex to ignore the 1 cases.

@faho
Copy link
Member Author

faho commented Oct 14, 2022

Using cs.github.com shows much more variety

Yeah, they want me to sign up for the waitlist, I'll just take your word for it that there are other cases.

In our code non-1 cases are a rarity.

Anyway: I still don't believe that making this more complicated helps. You tell people that "1" is true, and they'll use "1". Tell people that "true is one of true, yes, on, 1", they'll have to think about which one of them they want to use, and case-insensitive means you can get "yEs", which I see absolutely no reason for (we're not case-insensitive in our keywords or variable names or really a lot of other places) and which I would always want to correct, everywhere.

@mqudsi
Copy link
Contributor

mqudsi commented Oct 14, 2022

I'm (also) saying people already have these set from a time before we told them what to set it to. Their configs will break.

The results show a bunch of themes that people clone/copy/download internally set "yes" or "true" for the prompt status variable. Anyone using these is going to have breakage.

The oh-my-fish theming guide use yes, true, and 1 all over the place in their theming guide and tutorials.

The results show a whole bunch of these.


I'd be fine with limiting it to just the case-sensitive values 1, true, or yes as a compromise based off what I'm seeing in the search results.

These appear to be reasonably common and so supporting them makes life
easier.

Incidentally, these are also acceptable to git, so we don't really
need to document what that takes.
@faho faho changed the title git prompt: Only interpret a 1 as true for bools git prompt: Interpret values as true for bools instead of relying on defined-or-not Oct 15, 2022
@mqudsi
Copy link
Contributor

mqudsi commented Oct 15, 2022

It's nice that this clears up the earlier confusing statement about boolean variables vs git variables.

@faho faho merged commit 8c362c8 into fish-shell:master Oct 21, 2022
@faho faho deleted the git-prompt-bools branch October 21, 2022 18:24
planet36 added a commit to planet36/dotfiles that referenced this pull request Jan 20, 2023
planet36 added a commit to planet36/dotfiles that referenced this pull request Jan 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants