search

denoland / deno_install

Deno Binary Installer
https://deno.land/
MIT License
966 stars 179 forks source link

Make install script provide tips that are relevant to shell used by the user #261

Closed erelinz closed 1 year ago

erelinz commented 1 year ago

This PR adds two statements to the install script, the statements check if your using a different shell as your default (other than the shell used to run the install script) and would recommend that you add deno to the path based on that shell default config files.

cc: @MarkTiedemann

MarkTiedemann commented 1 year ago

@erelinz I'm not a maintainer. Just a contributor. Here's my opinion:

While your changes may be technically correct, I'm not sure the added complexity is warranted.

Consider the following lines you added:

dscl . -read ~/ UserShell | awk '{print $2}'
getent passwd "$(id -un)" | cut -d: -f7

The installer script should be kept simple - so simple that you can read it in one go and be sure that it's not gonna harm your system. I'm not sure that's the case with these two commands, especially with the second one referring to passwd. I don't think these are very common. So it won't be simple for most users to understand them.

Therefore, I'd rather not add these changes.

erelinz commented 1 year ago

Sorry @MarkTiedemann I saw you made lots of contributions and wrongly assumed. Please ignore.

@erelinz I'm not a maintainer. Just a contributor. Here's my opinion:

While your changes may be technically correct, I'm not sure the added complexity is warranted.

Consider the following lines you added:

dscl . -read ~/ UserShell | awk '{print $2}'
getent passwd "$(id -un)" | cut -d: -f7

The installer script should be kept simple - so simple that you can read it in one go and be sure that it's not gonna harm your system. I'm not sure that's the case with these two commands, especially with the second one referring to passwd. I don't think these are very common. So it won't be simple for most users to understand them.

I think the your logic is fair but not the conclusion. The purpose of this PR is to make it easier and simple for users to install deno, regardless of their OS and shell of choice.

Users should be able to understand what the script does if they chose to review it. If they are unsure, they can paste it into chatgpt or https://explainshell.com/ and they'll be be satisfied with the answer.

The script uses command line utilities that are installed in many distributions, so no additional installation of packages would required for using those commands. We could also add more comments explaining why we want to tailor the output to the user's OS and shell.

I've tested it on macOS other linux distributions:

docker run -it --name alpine alpine sh -c "apk update && apk add curl && curl -fsSL https://raw.githubusercontent.com/erelinz/deno_install/fish_aware_tip/install.sh |sh"
docker run -it --name ubuntu ubuntu bash -c "apt update && apt -y install curl unzip && curl -fsSL https://raw.githubusercontent.com/erelinz/deno_install/fish_aware_tip/install.sh |sh"

Similarly for macOS, dscl and awk are available by default.