-
Notifications
You must be signed in to change notification settings - Fork 760
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
rewrite asdf function to not rely on the "local" keyword #1138
Conversation
The `asdf` function makes use of the `local` keyword which is not supported by all shells. A simple rewrite of the function allows to eliminate the use of a (local) variable to store the first argument and, as a consequence, removes the need for the `local` keyword.
Is there a bug report for this issue behind this PR? It's not clear to me why this is needed. bash-3.2$ asdf if
Unknown command: `asdf if`
No plugin named if |
I'm reporting the bug and providing a patch at the same time.
Yes,
Full reproduction example:
I should note that the "proper" way to support |
So it seems like out of the box everything does work when invoked from |
This could have been argued right at the start, but would only have addressed
Most modern shells, in one form or another supports So there are 3 things here:
1 addresses 2 in a very specific way that entirely fixes my usage pattern, but I think it would be nice if 2 was addressed globally. That's up to asdf to decide if 2 is important considering ^^. If it is, 3 is a possible solution. You're welcome. Now this change is absolutely trivial and localized. It entirely fixes my use case and I would like it to be merged. But if you want to reject the PR entirely based on the fact that it doesn't apply in a default configuration, be my guest. In any case, I think I detailed the issue enough that it should be pretty clear even for someone uneducated on the issue and enough to make a proper decision going forward. Thank you for your time |
@hyperupcall what are your thoughts on this? |
@Stratus3D I'm all for these changes in general - I know I originally stated that I want to let |
@Stratus3D I think a PR like this should also include some regression testing. In this case, a version of the As @hyperupcall mentions, it may be covered by fixing #1397, which requires us to decide whether we want these entrypoint files to be considered as Bash or POSIX when linting. So depending on the outcome of #1397, we may not need the above-mentioned file-specific |
# commands that need to export variables | ||
eval "$(asdf export-shell-version sh "$@")" # asdf_allow: eval | ||
;; | ||
*) | ||
# forward other commands to asdf script | ||
command asdf "$command" "$@" # asdf_allow: ' asdf ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add # asdf_allow: ' asdf '
back in here.
Summary
The
asdf
function makes use of thelocal
keyword which is notsupported by all shells. A simple rewrite of the function allows to
eliminate the use of a (local) variable to store the first argument
and, as a consequence, removes the need for the
local
keyword.Looking at the code,
local
is used in a lot of other places, but thisis the only place where my shell (
mksh
) generates an error.Other Information
As per the
KSH_VERSION
environment variable, I usemksh
version@(#)MIRBSD KSH R59 2021/10/15 +Debian
. I don't use Docker now, but I'm assuming (untested) the following would create an environment to reproduce the behavior.