close
Skip to content

fix!: align Fish entrypoint behaviour with other shells#1524

Merged
jthegedus merged 4 commits into
asdf-vm:masterfrom
hyperupcall:salmon
Mar 30, 2023
Merged

fix!: align Fish entrypoint behaviour with other shells#1524
jthegedus merged 4 commits into
asdf-vm:masterfrom
hyperupcall:salmon

Conversation

@hyperupcall

@hyperupcall hyperupcall commented Mar 29, 2023

Copy link
Copy Markdown
Contributor

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
Comment thread scripts/shfmt.bash
Comment thread scripts/format.bash
test/*.bats

# check .fish files
fish_indent --write ./**/*.fish

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Copy Markdown
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.

@jthegedus jthegedus left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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