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: use PATH instead of fish_user_paths + tests #1709

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

bakotaco
Copy link
Contributor

Summary

Update asdf.fish to modify $PATH directly, rather than using $fish_user_paths. This respects the user's choice between a universal or global $fish_user_paths, addressing an issue where the previous asdf.fish made this decision on behalf of the user.

Modifying $PATH directly was recommended by @faho (long time Fish shell contributor) and avoids assumptions about $fish_user_paths being global or universal, which should be the shell users decision. Note that in fish $PATH cannot be a universal variable (it's simply inherited from the parent process), so it does not have this issue.

This approach also aligns with initialisation scripts from other projects, such as those from brew and pyenv that also set PATH directly, ensuring simplicity.

A side effect is that this also fixes tests and resolves test interdependencies caused by the persistence of the universal variable across runs.

Fixes:

@hyperupcall
Copy link
Contributor

Thanks for the fix! Since PATH is not a universal variable, this means that this code must be sourced on every fish shell initialization, right?

Also, it looks like you accidentally commited your Intelij files

@bakotaco
Copy link
Contributor Author

bakotaco commented Jan 16, 2024

Since PATH is not a universal variable, this means that this code must be sourced on every fish shell initialization, right?

Yes, that's correct: $PATH is a global variable, so asdf.fish will need to be sourced in every new fish shell session.

looks like you accidentally commited your Intelij files

Oops, removed

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation, cheers!

@jthegedus jthegedus merged commit 5327697 into asdf-vm:master Jan 18, 2024
7 checks passed
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