Unsolicited opinions on setup

halostatue commented 4 weeks ago

I saw your phx.tools update announcement recently on ElixirStatus and have a few observations that you may wish to consider.

Shell Support

Your setup configuration only supports bash and zsh; fish is an increasingly popular alternative shell which should be supported. The ideal way to do this is via a new file (~/.config/fish/conf.d/phx_tools.fish) and the contents should be mise activate fish | source. If you don't wish to support fish, then your documentation should be clear on that.
Modifications should be made to both ~/.bashrc and ~/.zshrc if either exists regardless of the current value of $SHELL (also to the fish config file, but only if fish is installed).
Modifications should be made to configuration files only if the modifications don't already exist. For bash, this would mean grepping in ~/.bash_profile, ~/.bashrc, etc.. For zsh this would mean grepping in ~/.zshrc etc. For fish, this would mean grepping in ~/.config/fish/config.fish and ~/.config/fish/conf.d/*. This is less problematic than it appears because the only update to the config file happens in maybe_install mise, but it is possible that someone already initializes mise elsewhere behind a test ([[ -x mise ]] && eval "$(mise activate bash)").

Platform Support

The only Linux distribution family supported is Debian. This should be documented (and eventually expanded; based on what I have seen, supporting dnf and pacman in addition to apt would be sufficient — maybe apk for minimal cases).

Maintenance

There is some divergence in your macOS and Linux scripts that I do not believe you have intended. I believe that you would be better served by serving a single script which supports both macOS and Linux. The differences between the implementations can either be done with "tagged" functions (maybe_install_macos, maybe_install_linux) or conditional definitions:

if is_macos; then
  maybe_install() {
    :
  }
else
  maybe_install() {
    :
  }
fi

I’m attaching a quick change that should support fish and fix some of the divergence that I mentioned above, but doesn't address anything else that I mentioned.

support fish

```diff diff --git i/priv/static/Linux.sh w/priv/static/Linux.sh index ea0bc6b764e4..1473977d459b 100755 --- i/priv/static/Linux.sh +++ w/priv/static/Linux.sh @@ -35,12 +35,17 @@ case "${SHELL:-}" in ;; */zsh) current_shell="zsh" config_file="$HOME/.zshrc" ;; +*/fish) + current_shell="fish" + config_file="$HOME/.config/fish/conf.d/phxtools.fish" + mkdir -p "$(dirname "$config_file")" + ;; *) - printf "Unsupported shell: $SHELL\n" + printf "Unsupported shell: %s\n" "$SHELL" exit 1 ;; esac already_installed() { @@ -62,11 +67,11 @@ already_installed() { ;; "PostgreSQL") mise which initdb >/dev/null 2>&1 ;; *) - printf "Invalid name argument on checking: $1\n" + printf "Invalid name argument on checking: %s\n" "$1" exit 1 ;; esac } @@ -98,10 +103,13 @@ install() { echo 'eval "$(~/.local/bin/mise activate bash)"' >>$config_file ;; "zsh") echo 'eval "$(~/.local/bin/mise activate zsh)"' >>$config_file ;; + "fish") + echo 'mise activate fish | source' >>$config_file + ;; esac export PATH="$HOME/.local/bin:$PATH" ;; "Phoenix") @@ -121,13 +129,13 @@ install() { esac } maybe_install() { if already_installed "$1"; then - printf "$1 is already installed. Skipping...\n" + printf "%s is already installed. Skipping...\n" "$1" else - printf "Installing $1...\n" + printf "Installing %s...\n" "$1" if [ "$1" = "Erlang" ]; then printf "This might take a while.\n" fi printf "\n" install "$1" diff --git i/priv/static/macOS.sh w/priv/static/macOS.sh index fb020836c238..9b0ff7bbfab8 100755 --- i/priv/static/macOS.sh +++ w/priv/static/macOS.sh @@ -35,10 +35,15 @@ case "${SHELL:-}" in ;; */zsh) current_shell="zsh" config_file="$HOME/.zshrc" ;; +*/fish) + current_shell="fish" + config_file="$HOME/.config/fish/conf.d/phxtools.fish" + mkdir -p "$(dirname "$config_file")" + ;; *) printf "Unsupported shell: %s\n" "$SHELL" exit 1 ;; esac @@ -112,10 +117,13 @@ install() { echo 'eval "$(~/.local/bin/mise activate bash)"' >>$config_file ;; "zsh") echo 'eval "$(~/.local/bin/mise activate zsh)"' >>$config_file ;; + "fish") + echo 'mise activate fish | source' >>$config_file + ;; esac export PATH="$HOME/.local/bin:$PATH" ;; "Phoenix") ```

almirsarajcic commented 4 weeks ago

Thank you for your thorough feedback and the solutions you've provided.

We've planned to combine the scripts into one to prevent any discrepancies.

I thought supporting fish would be too much work for this stage, so I skipped that, but with your help seems we'll be able to support it soon.

Regarding Linux distribution, if it's not too much work, we can support that. I'll have to figure out what to do about testing that.

halostatue commented 4 weeks ago

One other thought is that ~/.local/bin is not in $PATH by default, so you may want to add that before the mise configuration change unless the mise installation script does that (I’ve had ~/.local/bin in my $PATH for so long that I don't even notice it, and I am using sudo port install mise rather than brew install mise or the mise installation script).

For fish, the pattern to be added to the configuration file would be something like:

if ! command -sq mise
  fish_add_path -p -P $HOME/.local/bin
end

if command -sq mise
  mise activate fish | source
end

The latter should be blocked in a status is-interactive test (but this should be done for bash/zsh as well, with [[ $- == *I* ]]).

optimumBA / phx.tools