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

Add support for configurable extra fzf arguments #60

Closed
wants to merge 4 commits into from
Closed

Add support for configurable extra fzf arguments #60

wants to merge 4 commits into from

Conversation

lzybkr
Copy link
Contributor

@lzybkr lzybkr commented Apr 8, 2020

via the environment variable _ZO_FZF_EXTRA_ARGS

via the environment variable _ZO_FZF_EXTRA_ARGS
Copy link
Owner

@ajeetdsouza ajeetdsouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think the variable can safely be renamed to _ZO_FZF_ARGS without any loss in clarity.

As an aside, this is the second feature to require splitting logic due to the datatype being a list (along with #48), and it does make me wonder if switching to a TOML/YAML config file would be a good idea.

README.md Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
@cole-h
Copy link
Contributor

cole-h commented Apr 8, 2020

Somewhat off-topic for this PR, but related to @ajeetdsouza's earlier comment on configuration:

While TOML/YAML configuration might be nice, I think that env var configuration is nicer. Want to test something? _ZO_DATA_DIR=/tmp/zo zoxide add /tmp. Want to change the max age for a single shell session? export _ZO_MAXAGE=9999 (or however you achieve that in any arbitrary shell).

If we ship a default configuration file, we then have to make sure users know when new options are added, or options are deleted/deprecated. If we don't ship a default configuration, we have to make sure users know how to create their own. It also becomes another folder related to zoxide on users' filesystems: their local data dir for the database itself, and now the config dir for the configuration file. The file format itself also brings with it a whole host of issues, see: the many different "bugs" filed on Alacritty's issue tracker about configuration -- improper indentation, duplicate keys, unused keys, etc., in their case of YAML.

Overall, I much prefer having to use splitting logic over a separate config file, even if env vars were never truly removed.

(If there's still more to discuss on this topic, maybe it would be better to open an issue and ping me there, or even email me, to prevent cluttering this PR further.)

@lzybkr lzybkr requested a review from ajeetdsouza April 8, 2020 23:24
Copy link
Contributor

@cole-h cole-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget a CHANGELOG entry, please :) Maybe:

### Added
...
- Specify additional arguments to `fzf` with `$_ZO_FZF_ARGS`
...

README.md Outdated Show resolved Hide resolved
Copy link
Owner

@ajeetdsouza ajeetdsouza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than @cole-h's suggestions, LGTM

@lzybkr
Copy link
Contributor Author

lzybkr commented Apr 9, 2020

OK, hopefully this is ready now.

It might be nice to add a suggested setting for $_ZO_FZF_ARGS in the README.md but I haven't settled on a "perfect" suggestion yet. So far I'm liking:

$env:_ZO_FZF_ARGS = "--height 15% --layout=reverse --info=hidden --preview ""ls {2}"""

@cole-h
Copy link
Contributor

cole-h commented Apr 9, 2020

If we want to suggest anything at all, I'd rather go for something simple: _ZO_FZF_ARGS="--height 20%" (or 15% as you have it). I don't like the idea of suggesting something as complicated as yours -- if a user wants to tweak further, they should do research into what options/flags fzf supports.

@ajeetdsouza
Copy link
Owner

I think it would be best to add a link to the fzf documentation.

I also just checked the fzf docs, and it appears we have a solution to our splitting issue! Apparently, fzf uses $FZF_DEFAULT_OPTS, which we can use for our implementation.

I suggest:

  • We change the variable name to $_ZO_FZF_OPTS for consistency.
  • We use the Command::env function to set $FZF_DEFAULT_OPTS when starting fzf. This will also ensure that we do not clash with the user's $FZF_DEFAULT_OPTS, which they may be using for a different purpose.

@lzybkr
Copy link
Contributor Author

lzybkr commented Apr 9, 2020

I considered using $FZF_DEFAULT_OPTS but decided against that. The scenarios I considered:

  • User is using $FZF_DEFAULT_OPTS for UI settings - they probably wouldn't use $_ZO_FZF_OPTS.
  • User is using $FZF_DEFAULT_OPTS for other settings, e.g. history - we probably shouldn't override their settings.

@ajeetdsouza
Copy link
Owner

ajeetdsouza commented Apr 9, 2020

I can think of 2 solutions to this:

  1. _ZO_FZF_OPTS always overrides FZF_DEFAULT_OPTS. This means that an unset _ZO_FZF_OPTS will run fzf without any extra configuration. This keeps the behaviour for zoxide entirely separate, and if the user wants to keep the configuration the same, he can set _ZO_FZF_OPTS="$FZF_DEFAULT_OPTS".

  2. _ZO_FZF_OPTS overrides FZF_DEFAULT_OPTS only when it's set. This is less consistent, but it also means that fzf will work for the user out of the box as expected - and can be overridden with _ZO_FZF_OPTS="".

Of course, in either case, if the user wants default functionality and something extra, they could always go with _ZO_FZF_OPTS="$FZF_DEFAULT_OPTS --height 15%".

@lzybkr
Copy link
Contributor Author

lzybkr commented Apr 10, 2020

Having given this more thought, I think this PR isn't that useful.

If one wants to specify custom options, they should simply replace the appropriate aliases (zi, zqi, and zri) with their own alias/function/script (depending on their shell and intentions) that defines their preferred options, e.g. a bash user could use:

alias zi='FZF_DEFAULT_OPTS="--height 20" z -i'

@ajeetdsouza
Copy link
Owner

That is true. One could also alias zoxide itself to change the interactive mode of both zoxide query and zoxide remove. I'll close this PR.

@lzybkr lzybkr deleted the fzf_options branch April 10, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants