Skip to content

Conversation

@arsalanyavari
Copy link

Summary

This pull request includes two commits to improve the installation process.

  1. Install bash before FZF installation
    The first commit addresses the requirement for Bash when installing FZF. Some distributions, such as Alpine, do not include bash by default, and the current installation instructions do not specify this dependency.

  2. Update installation instructions without using a framework repository link
    The second commit modifies the installation instructions to accommodate users who may not have an SSH key configured for GitHub, allowing them to use the HTTPS link instead of the [email protected] format for cloning.

Changes

  • Added Bash installation step for FZF.
  • Updated repository cloning instructions to support HTTPS.

Thank you for considering this pull request!

if ! _fzf_has fzf; then
if [[ ! -d $FZF_PATH ]]; then
git clone --depth 1 https://github.com/junegunn/fzf.git $FZF_PATH
[ $(which bash) ] >/dev/null || apk add bash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work on alpine. Also, installing bash is out of scope for a ZSH plugin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. I was biased towards my own issue. Given this situation, do you think it would be beneficial to add the following code snippet?

if ! command -v bash >/dev/null 2>&1; then
    echo "Bash is not installed. Installing now..."

    if command -v apt >/dev/null; then
        sudo apt update && sudo apt install -y bash
    elif command -v dnf >/dev/null || command -v yum >/dev/null; then
        sudo dnf install -y bash || sudo yum install -y bash
    elif command -v zypper >/dev/null; then
        sudo zypper install -y bash
    elif command -v pacman >/dev/null; then
        sudo pacman -Sy --noconfirm bash
    elif command -v apk >/dev/null; then
        sudo apk add --no-cache bash
    elif command -v brew >/dev/null; then
        brew install bash
    elif command -v nix-env >/dev/null; then
        nix-env -iA nixpkgs.bash
    else
        echo "Unsupported OS. Please install Bash manually."
        exit 1
    fi
    echo "Bash installation completed."
fi

Or, if Bash isn't available, should there be a message printed to inform the user what action to take and prompt them to restart the installation process?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installing bash is out of scope. This plugin is designed to make fzf easy to use with ZSH frameworks, not to support multiple shells.

If someone doesn't have bash on their machine they've had to go out of their way to remove it, given that it's installed by default on most linux distributions.

Installing a shell that a user doesn't explicitly want on their machine feels wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants