cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.77k stars 176 forks source link

Use `source <(mcfly init *sh)` instead of `eval "$(mcfly init *sh)"` #254

Closed tombh closed 2 years ago

tombh commented 2 years ago

Really enjoying this project, thank you 💕

As mentioned in #219 and #239, there is unexpected behaviour when re-running .zshrc. As @dasebasto pointed out, this is because of:

if [[ "$__MCFLY_LOADED" == "loaded" ]]; then
  return 0
fi
__MCFLY_LOADED="loaded"

When return 0 is eval'd it returns from the whole .zshrc script, thus failing to run any further commands such as binding bindkey '^R' mcfly-history-widget, but more importantly failing to run the rest of .zshrc without warning.

Of course I totally agree that source ~/.zshrc is not best practice. But it is a practice. As such, it should not be unexpectedly broken without warning.

The quickest fix for this is just to update the README to suggest loading mcfly with source <(mcfly init zsh). Another idea is to add an intermediate step, for example this is how starship does it:

# Show contents
% starship init zsh
source <(/usr/bin/starship init zsh --print-full-init)
# Run contents
% eval "$(starship init zsh)"
🚀
cantino commented 2 years ago

Thank you for explaining this, I wasn't aware of that behavior of return 0. Should we just wrap the entire script in if ! [[ "$__MCFLY_LOADED" == "loaded" ]]; then ?

tombh commented 2 years ago

I think the script should remain unchanged, it's a good script. And anyway, it doesn't help you having to keep worrying about what you write there. Really the problem is the eval "$(mcfly init zsh)" in the README. Just updating the README with source <(mcfly init zsh) is enough.

But if you were up for it, the perfect solution would be what I suggested about an intermediate step. So that mcfly init zsh just returned source <(mcfly init zsh --print-full-init). And then mcfly init zsh --print-full-init was the command that actually output the script. That way, all existing users get to benefit from the new approach without having to lift a finger 🤓

cantino commented 2 years ago

Any chance you want to make this change? ;) Otherwise, I can soon.

Edit: and thanks for the clear explanation!

tombh commented 2 years ago

PR done!