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: Better handling with paths that include spaces #1485

Merged
merged 4 commits into from
Mar 24, 2023

Conversation

hyperupcall
Copy link
Contributor

Summary

I saw that there was a ASDF_BATS_SPACE_IN_PATH variable, which seemed to be part of a gradual transition to make the tests work better with whitespace stuff. Probably with help from #1433, it was somewhat trivial to make the proper fixes and remove this variable entirely.

One test was left out because it seems that change requires a fix in the .tool-versions parser (related to the path:~/src/elixir feature)

@hyperupcall hyperupcall requested a review from a team as a code owner February 21, 2023 01:46
@jthegedus
Copy link
Contributor

jthegedus commented Mar 24, 2023

The changes I requested seemed to have introduced a bunch of shellcheck issues we would need to ignore. Are you able to resolve them @hyperupcall ?

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Mar 24, 2023

The changes I requested seemed to have introduced a bunch of shellcheck issues we would need to ignore. Are you able to resolve them @hyperupcall ?

I wish I mentioned this earlier, but the reason why the Bats test fails is because the newer changes moves the BASE_DIR=... inside of the test function. This means that it has no effect since BASE_DIR is used in setup, which runs before the test function.

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Mar 24, 2023

I reverted those changes, and the BASE_DIR is now set inside of setup(), so things should work. I know it's kind of ugly since the condition depends on the name of the test and there is a separation of sorts - do you think there is a better way or are the changes OK as they are?

My first thought would be to move the invocation of setup_asdf_dir from within setup(), to each individual function, just for the tests in shim_exec.bats, but it seems that would make all that code harder to read and more repetitive just for the sake of a single test.

I also tried re-calling setup (with the new BASE_DIR) from within the test function, but that doesn't seem to work either.

@jthegedus
Copy link
Contributor

It is a shame we cannot opt-out of using the setup for a single test in the file. The most clean alternative I can think of is having a separate test file for these specific tests.

If someone thinks of something better, we can discuss then, in the meantime we will merge this.

@jthegedus jthegedus merged commit bbcbddc into asdf-vm:master Mar 24, 2023
@hyperupcall hyperupcall deleted the fix-test-space branch March 24, 2023 12:37
botp pushed a commit to botp/asdf that referenced this pull request Mar 31, 2023
Comment on lines +24 to +33
local is_nullglob_disabled=
shopt -q nullglob || is_nullglob_disabled=yes
shopt -s nullglob
for f in "$(asdf_data_dir)"/shims/*; do
if grep -q "asdf-plugin: ${plugin_name}" "$f"; then
rm -f "$f"
fi
done
[ "$is_nullglob_disabled" = 'yes' ] && shopt -u nullglob
unset -v is_nullglob_disabled
Copy link
Member

Choose a reason for hiding this comment

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

Is there really not a better way of doing this? This seems like quite a bit of code just to find and delete shims containing a certain line.

Copy link
Contributor Author

@hyperupcall hyperupcall Apr 5, 2023

Choose a reason for hiding this comment

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

There was, https://github.com/hyperupcall/asdf/blob/82b9ef01e26f39b375c1b462979aa7ffd069df8a/lib/commands/command-plugin-remove.bash#L24 but it didn't work on Alpine.

I suppose I could have replaced the nullglob stuff with a [ -f "$f" ] in the loop

Copy link
Member

Choose a reason for hiding this comment

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

I guess I'd prefer [ -f "$f" ] in the loop as it feels harder to get wrong - it's easy for someone to forget to unset an option or reset it to the wrong value. In this case it looks correct so not an issue.

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.

3 participants