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!: align Fish entrypoint behaviour with other shells #1524

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

hyperupcall
Copy link
Contributor

@hyperupcall hyperupcall commented Mar 29, 2023

Summary

As per this comment, this matches the behavior of Fish with Bash, Nushell, and all the other shells. Along with a few other things

There were some bugs with the previous Fish implementation that are now fixed:

  • If ASDF_DIR was set to an empty string, the value was used (unlike in Bash/Zsh, Elvish, Nushell, which replaces it with a correct one)
  • ASDF_DIR is now always an absolute path to the asdf directory (previously it could have been a .). (Since it's an absolute path, we now export it so that asdf can use it instead of recalculating)
  • Depending on version, either PATH or fish_user_paths (through fish_add_path) would be modified. Now, only the latter is potentially modified
  • Flag forms to the status builtin like -f are now deprecated in Fish and have been replaced with the alternative (string filename)

It seems that fish_add_path potentially modifies the placement of an entry in $fish_user_paths, even if --move is not specified, so I opted to prepend to $fish_user_paths manually, so the behavior is consistent with all the other shells.

Now we also use fish_indent to make sure Fish scripts are formatted nicely (even if we have relatively few of them)

@hyperupcall hyperupcall requested a review from a team as a code owner March 29, 2023 01:16
@hyperupcall hyperupcall changed the title Salmon Fish Entrypoint fixes Mar 29, 2023
@hyperupcall hyperupcall changed the title Fish Entrypoint fixes fix!: Fish Entrypoint fixes Mar 29, 2023
@@ -23,3 +23,6 @@ shfmt --language-dialect bash --indent 2 --write \
# check .bats files
shfmt --language-dialect bats --indent 2 --write \
test/*.bats

# check .fish files
fish_indent --write ./**/*.fish
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is run locally, it would be ideal to check Fish is available or error with a warning to the user that it is required.

Even more ideal would be to get fish_indent as a standalone CLI tool and manage it with our .tool-versions file so people get the same experience as with shellcheck and shfmt in this repo.

A separate Issue tracking this would suffice I would say.

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.

I don't use fish anymore so haven't tested this locally myself, but LGTM

@jthegedus jthegedus changed the title fix!: Fish Entrypoint fixes fix!: align Fish entrypoint behaviour with other shells Mar 29, 2023
@jthegedus jthegedus merged commit 8919f40 into asdf-vm:master Mar 30, 2023
@jthegedus jthegedus mentioned this pull request Mar 30, 2023
@jthegedus
Copy link
Contributor

Merging to get people testing this.

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.

2 participants