kutsan / zsh-system-clipboard

System clipboard key bindings for Zsh Line Editor with vi mode. It is similar to what `set clipboard=unnamed` does for vim.
GNU General Public License v3.0
149 stars 16 forks source link

[RFC] Use aliases instead of functions for set and get #15

Closed doronbehar closed 6 years ago

doronbehar commented 6 years ago

Better readability, safer since no command line arguments are passed between the functions and only stdin and stdout are used.

kutsan commented 6 years ago

Thanks again but to be honest I don't think this helps readability at any point, rather it is difficult to read in my opinion. Using function is easy and understandable. Tell me your thoughts, I'm open to suggestions. If you still think this is the correct way to do that, then I'll accept the PR in no-time.

doronbehar commented 6 years ago

Well when I first started crafting this PR I wanted to prepare the ground for a bigger PR which will fix an issue which has been buggering me for a while. This PR was meant to make sure no spaces or newlines are trimmed before the clipboard is set.

I've finished preparing the next PR and I have investigated further more $* and $@ in shells and I have even tested it with the functions instead of the aliases and no differences in behaviour were seen.

Never the less, I still think this is much more readable and maintainable since it's hard to know what should be in $@ or $* in _zsh_system_clipboard_set and using just stdin or stdout with aliases is rock solid in comparison to functions which need to be evaluated as well. Plus, there's a small performance improvement given with this patch since $ZSH_SYSTEM_CLIPBOARD_TMUX_SUPPORT is not read every time the clipboard is set, I think it's a good enough reason to merge this but I'd like to hear your reply.

kutsan commented 6 years ago

Good points! In that aspect, those are valid reasons to merge this PR in. There are no words left to speak, it's merged. Thank you so much Doron.

doronbehar commented 6 years ago

Thanks! Wait for the next PR..