Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow path: versions to use ~ #1403

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Dec 27, 2022

Summary

$ asdf plugin-add elixir https://github.com/asdf-vm/asdf-elixir.git
$ asdf install elixir 1.10.2
$ asdf local elixir path:~/.asdf/installs/elixir/1.10.2/
$ cat .tool-versions
elixir path:~/.asdf/installs/elixir/1.10.2/
$ command -v elixir
/home/edwin/.asdf/shims/elixir

Closes: #941

Other Information

The lib/functions/versions.bash change is due to a bug that apparently didn't show up before (from this):

asdf local elixir path:~/.asdf/installs/elixir/1.10.2/
tail: cannot open '/tmp/tmp.OU7v1JCUaV/.tool-versions' for reading: No such file or directory

@hyperupcall hyperupcall requested a review from a team as a code owner December 27, 2022 22:31
@hyperupcall hyperupcall force-pushed the allow-tilda-in-path branch 2 times, most recently from a0e048f to 4904395 Compare December 27, 2022 22:39
lib/utils.bash Outdated
util_resolve_user_path "${version_info[1]}"
local full_version="${REPLY}"

printf "%s\n" "$full_version"
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't util_resolve_user_path print the resolved path directly? If it did we could simplify this to:

printf "%s\n" "$(util_resolve_user_path "${version_info[1]}")"

Copy link
Member

Choose a reason for hiding this comment

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

@hyperupcall based on your latest comment, could this be changed to:

util_resolve_user_path "${version_info[1]}"
printf "%s\n" "${REPLY}"

I'd also prefer the REPLY variable be named something more specific, so as to better indicate it's origin. In this case, what about $util_resolve_user_path_reply?

util_resolve_user_path "${version_info[1]}"
printf "%s\n" "${util_resolve_user_path_reply}"

Copy link
Contributor Author

@hyperupcall hyperupcall Jan 6, 2023

Choose a reason for hiding this comment

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

I feel very strongly towards REPLY - I did change it to util_resolve_user_path_reply though

lib/utils.bash Outdated Show resolved Hide resolved
Copy link
Member

@Stratus3D Stratus3D left a comment

Choose a reason for hiding this comment

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

Not sure about the REPLY variable. If we do want to use a variable I think it should have a more unique name, for example, util_resolve_user_path_reply.

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Dec 29, 2022

@Stratus3D I remember seing your post on ASDF performance measurements, and my takeaway was that performance in ASDF could be better for reshim, but also just in general. And I found the REPLY pattern to be extremely useful to preserve the performance in all of my Bash scripts, and I thought it would be useful here too.

It obviates the spawning of extraneous subshells, which in my experience solves the death by a thousand paper cuts that Bash performance usually boils down to (if you have different experience, I'd like to hear it!). The only downsides that I've come across is that you have to write the unset -v REPLY; REPLY= boilerplate, and that for extremely large functions, you may have to name REPLY variable to something else, or move the REPLY= stuff to the end of the function (depending on how you write it). If you wish, I've written more about REPLY here (it's a little out of date, but its correct enough).

But I do use it over named variables like the one you suggested because it is obvious what the return variable is (no need to check), since as you know Bash doesn't support returning actual values. It also matches the read and select builtins.

But for whatever reason, you don't like this pattern or it's not applicable as much here, then I can rewrite it to use the more traditional approach - what are your thoughts?

@Stratus3D
Copy link
Member

@hyperupcall that all makes sense, I'm just wondering if REPLY is the best choice of name. It's a stylistic thing I guess.

@Stratus3D
Copy link
Member

Looks like we encountered a schedule brownout for runners on Ubuntu 18.04. We'll need to address that at some point.

@hyperupcall did you see my PR comments above?

@hyperupcall hyperupcall force-pushed the allow-tilda-in-path branch 4 times, most recently from b07fd9b to d7b9767 Compare January 6, 2023 01:21
@hyperupcall
Copy link
Contributor Author

@hyperupcall did you see my PR comments above?

Yup! Fixed the things

@@ -64,7 +64,7 @@ version_command() {
sed -i.bak -e "s|^$plugin_name .*$|$plugin_name ${resolved_versions[*]}|" "$file"
rm -f "$file".bak
else
# Add a trailing newline at the end of the file if missing and file present
Copy link
Member

Choose a reason for hiding this comment

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

This change could probably be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops I don't think I intentionally removed that

@Stratus3D Stratus3D merged commit 670c96d into asdf-vm:master Jan 6, 2023
@hyperupcall hyperupcall deleted the allow-tilda-in-path branch May 29, 2023 16:51
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.

path: versions can't contain ~, lack documentation
2 participants