fzf path should be /usr/local/bin only in shared scenarios

viasite-ansible / ansible-role-zsh

Setup antigen with oh-my-zsh, powerlevel10k theme, fzf, autosuggestions, syntax-highlighting

https://habrahabr.ru/post/323496/

MIT License

347 stars 85 forks source link

fzf path should be /usr/local/bin only in shared scenarios #35

Closed higidi closed 5 years ago

higidi commented 5 years ago

… otherwise it should be installed in local user home

popstas commented 5 years ago

@higidi, good idea. I think that zsh_fzf_path should be configurable easily without edit vars/main.yml:

zsh_fzf_path should placed in default.yml
zsh_fzf_path can be redefined in runtime
zsh_path should use zsh_fzf_path
$HOME/bin is more common path that $HOME/.bin

Please, check my fixes: https://github.com/viasite-ansible/ansible-role-zsh/commit/5b9ab7aa3e94e815473adab684e612e45ed9ce9e

Are you agreed?

I will check tests and other pull requests in few days.

higidi commented 5 years ago

@popstas

Basically it is ok for me to place the zsh_fzf_path within the default yaml, i decided to put it into the vars file because of all other pathes were already placed there. One thing i don't understand is, why you hard overwrite the variable if you use the role in shared scenarios (zsh_shared: yes). Can you explain it to me?
Both .bin or bin is fine for me, i preffer .binbecause its not shown in the default directory listing, but both is ok for me.

All other changes in 5b9ab7a are fine for me.

popstas commented 5 years ago

vars.yml contains only computed variables that user should not want to change. If I prefer ~/bin and you prefer ~/.bin, we should add option to override variable without role fork.

I override variable in runtime mostly for backward compatibility. Another solution: add one more variable zsh_fzf_path_shared.

Personally, I have servers with 10-50 users with zsh_shared configs and root with personal config.

Future plan:

I close this PR without merge
I add zsh_fzf_path_shared and fix tests later today
You rebase your other PR's
I merge it

It's fine for you?

higidi commented 5 years ago

No, please don't add a variable to set this to .bin or bin. I'm sure i will arrange with that.

All other plans are fine for me.

Closed.