Unsolicited opinions on setup

[halostatue]

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 --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]

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]

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* ]]).

[almirsarajcic]

Resolved maintenance issue by combining scripts in #21.
Working on other stuff.

[almirsarajcic]

Resolved these in #21, #23, and #25.

Thanks @halostatue for the suggestions and code snippets!

[halostatue]

I have a few more comments following the latest updates.

• Regarding fish support (Support fish shell #23), it is not generally considered good form for tools to modify ~/.config/fish/config.fish, but instead to create a new file in ~/.config/fish/conf.d (e.g., ~/.config/fish/conf.d/phx_tools.fish). Even there, it's better to put things like mise activate behind if status --is-interactive conditions.

• A new file in a common place (~/.local/share/phx_tools/) is probably better for all shells for multiple reasons:

  • It is self-contained. That is, it can be simply installed on the end-user's system without modification.
  • It can be added to configuration files manually, if wanted.
  • It can be updated without having to worry about safely editing core user files.
  • Bash and zsh can generally source the same files (there are some differences, but not generally at the level that phx.tools is set up at).

[almirsarajcic]

I'm not sure how to address this.
I was using your suggestions and went with this:

diff --git a/priv/script.sh b/priv/script.sh
index 03f228e..33a243f 100755
--- a/priv/script.sh
+++ b/priv/script.sh
@@ -31,16 +31,18 @@ postgres_version=15.1
 case "${SHELL:-}" in
 *bash)
     current_shell="bash"
-    config_file="$HOME/.bashrc"
+    config_dir="$HOME/.config/phx_tools"
+    config_file="$config_dir/shell.sh"
     ;;
 *fish)
     current_shell="fish"
-    config_file="$HOME/.config/fish/config.fish"
-    mkdir -p "$(dirname "$config_file")"
+    config_dir="$HOME/.config/fish/conf.d"
+    config_file="$config_dir/phx_tools.fish"
     ;;
 *zsh)
     current_shell="zsh"
-    config_file="$HOME/.zshrc"
+    config_dir="$HOME/.config/phx_tools"
+    config_file="$config_dir/shell.sh"
     ;;
 *)
     printf "Unsupported shell: %s\n" "$SHELL"
@@ -48,6 +50,8 @@ case "${SHELL:-}" in
     ;;
 esac
 
+mkdir -p "$config_dir"
+
 # Add OS detection
 OS="$(uname -s)"
 
@@ -219,14 +223,21 @@ install() {
         curl https://mise.run | sh
 
         case $current_shell in
-        "bash")
-            echo 'eval "$(~/.local/bin/mise activate bash)"' >>$config_file
-            ;;
         "fish")
-            echo '~/.local/bin/mise activate fish | source' >>$config_file
+            echo 'if status --is-interactive' > "$config_file"
+            echo '    ~/.local/bin/mise activate fish | source' >> "$config_file"
+            echo 'end' >> "$config_file"
             ;;
-        "zsh")
-            echo 'eval "$(~/.local/bin/mise activate zsh)"' >>$config_file
+        *)
+            # Shared config for bash/zsh
+            echo '[ -x ~/.local/bin/mise ] && eval "$(~/.local/bin/mise activate bash)"' > "$config_file"
+
+            # Add source line to respective rc files if not already present
+            for rc_file in ~/.bashrc ~/.zshrc; do
+                if [ -f "$rc_file" ] && ! grep -q "source.*phx_tools/shell.sh" "$rc_file"; then
+                    echo "source $config_file" >> "$rc_file"
+                fi
+            done
             ;;
         esac
 

but mise activate line also contains shell name which, I guess, can't be the same for all of these.
What do you think? Feel free to open a PR.
Thank you for all your contributions so far.

[halostatue]

I’ll open a draft PR with a sketch of it.