-
-
Notifications
You must be signed in to change notification settings - Fork 192
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 Issue #367: Value of $_ overwritten in bash #381
base: master
Are you sure you want to change the base?
Conversation
I don't think how we hook in |
@@ -121,6 +121,12 @@ function test_chruby_auto_invalid_ruby_version() | |||
"$expected_auto_version" "$RUBY_AUTO_VERSION" | |||
} | |||
|
|||
function test_chruby_auto_preserves_dollar_underscore() | |||
{ | |||
: expected |
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.
Might want to be
: expected 1 2 3
assertEquals "value of $_ does not change" "$_" "expected 1 2 3"
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.
It would be just 3
as it's the last argument only...
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.
The :
is just a dummy command - one that returns true and has the side-effect of setting $_
.
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.
Could this be rewritten as explicit code that sets $_
or returns an a return value?
@postmodern how should I deal with that in this PR? |
@postmodern Merry Festivus. Where to from here? |
@@ -22,12 +23,13 @@ function chruby_auto() { | |||
chruby_reset | |||
unset RUBY_AUTO_VERSION | |||
fi | |||
: "$old_" # restore $_ (last argument of last command executed) | |||
} | |||
|
|||
if [[ -n "$ZSH_VERSION" ]]; then | |||
if [[ ! "$preexec_functions" == *chruby_auto* ]]; then | |||
preexec_functions+=("chruby_auto") |
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.
I believe zsh also supports the $_
variable.
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.
It does, but it does not suffer from this issue.
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.
I think it may be time to create a special chruby_auto_bash
function to wrap the Bash-specific logic.
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.
FYI, the save/restore $_
also works in zsh
, even though it is not required as zsh
saves/restores automagically.
@HaleTom so #!/bin/sh trap 'echo "Command: $BASH_COMMAND"' DEBUG func() { echo 'inside of a function' } echo 'outside of a function' func |
Adding this, the output is:
I'm not sure why I'm having a brain-fart: how do I run the tests again? (I couldn't see anything in Let me know if I can help out with anything from here. |
@postmodern Here is the code you want:
Please let me know what the next step is. |
@postmodern, @steakknife, |
Just checking in on this as it's been one human gestation since the last comment. What is the next step here? @postmodern @steakknife |
I see that Travis on Apple didn't seem to run correctly - it has null output. Is this expected? Any other blockers to merging? |
@HaleTom Apple (still) ships with Bash 3, so unfortunately all code has to be Bash 3 compatible. I have since switched to GitHub Actions, however I still need to fix how the test suite downloads/installs the macOS test ruby, which must be compatible with GitHub Action version of macOS. |
Fix Issue #367 based on this answer