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
[Merged by Bors] - Add fish support to install.sh #2389
Conversation
Thanks for contribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as expected using bash
and zsh
, but it doesn't seem to work at all with fish
.
In each of my examples, the user has the variant shell set as their login, to ensure the $SHELL
var is set correctly. This should always return the same value even if you were to pipe the script to a different shell.
bash
bashy@2ad9c674bbea:~$ echo $SHELL
/bin/bash
bashy@2ad9c674bbea:~$ cat install.sh | bash
[...]
fluvio: echo 'export PATH="${HOME}/.fluvio/bin:${PATH}"' >> ~/.bashrc
zsh
2ad9c674bbea% echo $SHELL
/usr/bin/zsh
2ad9c674bbea% cat install.sh|zsh
[...]
fluvio: echo 'export PATH="${HOME}/.fluvio/bin:${PATH}"' >> ~/.zshrc
For me, the script has an error if I try to execute w/ fish
fishy@2ad9c674bbea ~> echo $SHELL
/usr/bin/fish
fishy@2ad9c674bbea ~> cat install.sh | fish
fish: Unexpected ')' found, expecting '}'
x86_64-unknown-linux-gnu)
^
This would print fish_hint
, but there's a little typo.
fishy@2ad9c674bbea ~ [0|127]> cat install.sh | bash
[...]
fluvio: echo 'export PATH="${HOME}/.fluvio/bin:${PATH}"' >> ~/.bashrc
fishy@2ad9c674bbea ~> cat install.sh | zsh
[...]
fluvio: echo 'export PATH="${HOME}/.fluvio/bin:${PATH}"' >> ~/.bashrc
When I changed L159 to fish_hint
, this does print correctly, however.
fishy@2ad9c674bbea ~> cat install.sh | zsh
[...]
fluvio: fish_add_path "$HOME/.fluvio/bin"
The shell is detected based on the bin name, and remind_path reverts to the previous behaviour (print all hints) if the detection fails.
Makes it a little easier to see which part to copy/paste in the fallthrough case (where shell can't be detected).
06b4bba
to
ba40b23
Compare
can you add to changelog? |
Sure. I didn't make an issue like the other entries - just link to this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. thanks
bors r+ |
1 similar comment
bors r+ |
I recently installed Fluvio from the online installer, but I felt left out when it came time to adding the bindir to my path. The first commit is the important one, the others just clean the output up slightly, now there are 3 variants.
Pull request successfully merged into master. Build succeeded: |
I recently installed Fluvio from the online installer, but I felt left out when it came time to adding the bindir to my path.
The first commit is the important one, the others just clean the output up slightly, now there are 3 variants.