Provide trap function for wrapping DEBUG signal traps #227

Open
aprescott opened this Issue Dec 2, 2013 · 3 comments

Comments

Projects
None yet
3 participants

Occasionally .bashrc (or similar) has an existing trap 'foo' DEBUG which can conflict with auto.sh's. I recently hit this problem since I noticed .ruby-version was being ignored.

The way I fixed this was to patch auto.sh to have:

function chruby_trap() {
    [[ "$BASH_COMMAND" != "$PROMPT_COMMAND" ]] && chruby_auto
}

if [[ -n "$ZSH_VERSION" ]]; then
    if [[ ! "$preexec_functions" == *chruby_auto* ]]; then
        preexec_functions+=("chruby_auto")
    fi
elif [[ -n "$BASH_VERSION" ]]; then
    trap chruby_trap DEBUG
fi

Then in my .bashrc, after loading chruby.sh and auto.sh, after any other calls, I provide my own trap:

# Reset color for command output and call this new `chruby_trap` function
trap 'foo_bar; chruby_trap' DEBUG

Obviously chruby_auto would also have worked, if I'd provided the explicit $BASH_COMMAND != $PROMPT_COMMAND check, but I figured it was possible that "setting up the trap" wouldn't necessarily always be the same as "call chruby_auto", hence the extra function.

I can submit a PR if you're happy with the idea (probably with refinements). Failing that, I can also submit a README note to mention that auto.sh will set a DEBUG trap and explain how to wrap it with chruby_auto.

(README update would also happen if chruby_trap is implemented of course! Or a wiki entry. Whichever.)

Contributor

ilikepi commented Jan 5, 2016

I happened upon this issue in the process of writing up a similar proposal. I agree it would be beneficial for the call to chruby_auto to be wrapped in another function that could more easily be overridden. I was going to propose a slightly different change however:

diff --git a/share/chruby/auto.sh b/share/chruby/auto.sh
index 88f3cb3..e3ad1ee 100644
--- a/share/chruby/auto.sh
+++ b/share/chruby/auto.sh
@@ -1,5 +1,9 @@
 unset RUBY_AUTO_VERSION

+function ruby_version_autoexec() {
+   chruby_auto
+}
+
 function chruby_auto() {
    local dir="$PWD/" version

@@ -25,9 +29,9 @@ function chruby_auto() {
 }

 if [[ -n "$ZSH_VERSION" ]]; then
-   if [[ ! "$preexec_functions" == *chruby_auto* ]]; then
-       preexec_functions+=("chruby_auto")
+   if [[ ! "$preexec_functions" == *ruby_version_autoexec* ]]; then
+       preexec_functions+=("ruby_version_autoexec")
    fi
 elif [[ -n "$BASH_VERSION" ]]; then
-   trap '[[ "$BASH_COMMAND" != "$PROMPT_COMMAND" ]] && chruby_auto' DEBUG
+   trap '[[ "$BASH_COMMAND" != "$PROMPT_COMMAND" ]] && ruby_version_autoexec' DEBUG
 fi

My own motivation is that I have another function I want to call after chruby_auto. Initially I was sourcing a second file with a modified trap command, but the second call to trap was ignored unless I first removed the existing trap command. This is rather awkward and confusing; having a wrapper function I can override seems more straightforward to me.

I had some difficulty settling on a name for the wrapper function. I got it in my head that it might make sense to give it a generic-sounding name as opposed to using the chruby_* prefix. I don't have a strong argument to support this, however.

Naming aside, I think this makes it easier to augment chruby_auto's behavior without having to patch auto.sh. I'm hoping to find some support for this idea...

ilikepi added a commit to ilikepi/chruby that referenced this issue Jan 7, 2016

HaleTom commented Apr 23, 2017

I also believe that chruby should not clobber an existing DEBUG trap, following the principle of least surprise.

This code allows extending an existing trap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment