wfxr / tmux-fzf-url

🚀 Quickly open urls on your terminal screen!
Other
526 stars 42 forks source link

macOS's default bash compatibility #23

Closed gavvvr closed 2 years ago

gavvvr commented 2 years ago

Hi @wfxr

I think you've started using modern bash's builtin (mapfile) just to fix linter's suggestions in 5b202610.

Since I've spent some significant time today before I realized that the plugin does not work just because of the default Mac's bash (and also since I do not want to install another version of bash, I just use Mac's default shell: zsh), I've decided to check if your awesome plugin can be made compatible with an old bash. I've checked the code and ensured that mapfile is the only feature which requires modern bash for tmux-fzf-url to work.

I am not a shell scripting expert, but I think the behavior will stay exactly the same if the following:

mapfile -t urls < <(echo "$content" |grep -oE '(https?|ftp|file):/?//[-A-Za-z0-9+&@#/%?=~_|!:,.;]*[-A-Za-z0-9+&@#/%=~_|]')

will be simply replaced with:

urls=$(echo "$content" |grep -oE '(https?|ftp|file):/?//[-A-Za-z0-9+&@#/%?=~_|!:,.;]*[-A-Za-z0-9+&@#/%=~_|]')

and shellcheck will not complain.

You anyway do not expect urls with whitespaces and looks like you do not care about them even if it was possible (because you further read an array as "${urls[@]}" instead of accessing it's elements by index to respect potential elements with whitespaces).

Please review this change and consider merging it if it does not break anything.

wfxr commented 2 years ago

@gavvvr Thank you! I think it's a good improvement!

gavvvr commented 2 years ago

Should mapfile in this line be replaced too?

Yes. Good catch, i've missed that one. Updated the PR + tested with bash 3.x on Mac

wfxr commented 2 years ago

@gavvvr LGTM. Thanks for your contribution!