search

sdkman / sdkman-hooks

An API responsible for serving up pre- and post- hooks
Other
5 stars 23 forks source link

replace `which` with `command -v` #20

Closed przemek-pokrywka closed 3 years ago

przemek-pokrywka commented 3 years ago

for all systems but Solaris (I'm not sure if it supports command -v).

That should fix https://github.com/sdkman/sdkman-cli/issues/759 and in general be nicer for people who build docker images, because command -v is better supported than which

przemek-pokrywka commented 3 years ago

The docker login seems to be failing during the CI build, does anyone need to update the credentials in the project setup?

helpermethod commented 3 years ago

Hi @przemek-pokrywka!

Sorry for letting you wait so long for a review.

PR looks good, but I would actually prefer

if ! command -v unzip > /dev/null; then

to

if [ -z $(command -v unzip) ]; then

Looks a little bit cleaner to me and does not require a command substitution.

przemek-pokrywka commented 3 years ago

The shell conditional statement keyword "if" relies on the exit code of the following command, so I think that the proposed change would invert the logic. Otherwise I have no strong opinions on the syntax used, it boils down to reader habits mostly. I probably won't have time to look at it but feel free to merge the changes with all amendments you would deem beneficial.

helpermethod commented 3 years ago

You are right, it mostly boils down to personal preference. I'll see if I can merge your changes as-is.

Thank you for your contribution!

przemek-pokrywka commented 3 years ago

Hi @helpermethod, I've managed to look at the change you've introduced and I recalled a possible reason for me having relied on the exit code (instead of [ -z). I may be wrong but I think that the behavior of command -v standard output differed per system and only the exit command was reliable. So you may want to double check that before doing a release.

helpermethod commented 3 years ago

@przemek-pokrywka Thank you for for mentioning this. I may just change the logic to rely on the exit code, just to be on the safe side 👍 .