Skip to content
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

Boolean option syntax change from set option/nooption/option! to set option true/false/toggle #758

Open
gokcehan opened this issue Feb 12, 2022 · 5 comments
Labels

Comments

@gokcehan
Copy link
Owner

Our current boolean option syntax is borrowed from vim which is confusing for some users (see #742 as an example). I still see many users in the wild having things like set hidden true in their config files which only works accidentally (see Brodie Robertson's config). This issue is worsened by the fact that our parser silently accepting such inputs as the option syntax is set option value in general. Having "on/off" for boolean default values in our documentation does not help this issue either. We could report such errors in our evaluator but it still feels like a band-aid solution.

So I propose we simply change the syntax of boolean options as follows:

set option   => set option true
set nooption => set option false
set option!  => set option toggle

An example with hidden option:

set hidden   => set hidden true
set nohidden => set hidden false
set hidden!  => set hidden toggle

A few remarks on this change:

  • I think it was a mistake expecting our users to be necessarily familiar with vim. Also, rest of the option setting syntax do not follow vim conventions anyway (e.g. we don't have set option=value and the rest).
  • I think the new syntax is more commonly used among other tools. ranger also uses this syntax except for toggling.
  • The new syntax is more regular and consistent with the rest of the options which should allow easier error checking.
  • The new syntax is compatible with our option exporting (e.g. !echo $lf_hidden already shows true/false).

We should be able to implement this change with a deprecation strategy by allowing both syntaxes to coexists for a few releases with a deprecation warning for the old syntax.

@gokcehan gokcehan pinned this issue Feb 12, 2022
@laktak
Copy link
Contributor

laktak commented Feb 16, 2022

I'm a vim user but I ran into this as well. Whatever way you go with this it would be helpful to mention the option syntax at the top of the options section. Or just link to syntax.

Somewhat related:

map " command

will cause parsing issues (should be map "\""). Is " supposed to be multiline?

@gokcehan
Copy link
Owner Author

@laktak I don't remember considering multiline strings when implementing quotes. I don't know if it is working and if it is working I'm not aware if someone has a use case for it. It might be a good idea to report such cases. Also worth mentioning that single quotes can also be used in this case (i.e. map '"' command).

@laktak
Copy link
Contributor

laktak commented Feb 19, 2022

@gokcehan I think my comment was too vague - map " will cause all (or most) options after this line to be ignored. Took me a while to notice that half my config is not working and I didn't immediately remember what I changed.

@gokcehan
Copy link
Owner Author

I had given up on this proposal as I thought it was going to be too painful to change the syntax at this point. After some thought, I decided that we can instead keep both syntaxes at the same time without removing the old ones. So I have now pushed a commit to accept the following forms:

set option true   # boolean enable
set option false  # boolean disable

To be specific, first one was already working accidentally. The second one is the breaking change (i.e. it used to enable the option instead of disable).

I haven't added anything new for toggling but only error handling. I don't think the original proposal was a good idea as it is a new invented syntax that we don't see users try by accident (i.e. set option toggle). If anything, maybe we can add something like set option !option to mimic how people write such logic in the code.

@jackielii
Copy link
Contributor

I was bitten by this a few days ago. (with set preview on/off just by reading the doc). Remember I found about about the preview/nopreview situation from someone else's config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants