Wrap chruby_auto to ease augmentation (issue #227)

[ilikepi]

This change implements the proposal in #227 and likely addresses #346 as well. All tests pass on OS X 10.10.

It occurs to me that this is somewhat of a breaking change for zsh users who expect chruby_auto to be added directly to preexec_functions (for example, the code in this comment from issue #191).

[aprescott]

It's been so long that a lot of this has seeped out of my head by now, but I was just re-reading my comment at #227 (comment) and thinking about the difference between calling

chruby_auto

and

[[ "$BASH_COMMAND" != "$PROMPT_COMMAND" ]] && chruby_auto

As I understand it, this change would repeatedly call chruby_auto without the initial "$BASH_COMMAND" != "$PROMPT_COMMAND" check. Does that have any side-effect when using bash?

[ilikepi]

@aprescott In my proposed change, the check of $PROMPT_COMMAND is still present within its original location in the DEBUG trap (line 36). The check prevents the trap from executing when $PROMPT_COMMAND itself executes. If the check weren't there and the user had $PROMPT_COMMAND set, the trap code would run twice:

$ export PROMPT_COMMAND='echo prompt'
prompt
$ pwd
/Users/ilikepi
prompt
$ function saytrap() {
>   echo trap
> }
prompt
$ trap 'saytrap' DEBUG
trap
prompt
$ pwd
trap
/Users/ilikepi
trap
prompt

I believe the check is most appropriate within the trap rather than in the wrapper function. If the check were moved to within the wrapper, that could easily lead to issues when a user tried to override the wrapper.

[HaleTom]

+1 for this PR.

I set my own DEBUG trap (to time each shell command), and it gets ugly if not all contained in one (well named) function.

Are there any objections to this? (I note it's over a year old)

[postmodern]

How about providing a common chruby_hook function that provides an entry-point for any/all hooks that run after every command, which auto.sh adds to.

[FranklinYu]

How about providing a common chruby_hook function that provides an entry-point for any/all hooks that run after every command, which auto.sh adds to.

Isn’t that exactly preexec?

[postmodern]

Isn’t that exactly preexec?

Unfortunately Bash (in particular Bash 3 which macOS still ships) does not have preexec.

[FranklinYu]

Then I’m once again advertising for https://github.com/rcaloras/bash-preexec, which saves us from manually fiddling with DEBUG trap (which is difficult to get right, anyway). It also fixes #367 for free: https://github.com/rcaloras/bash-preexec/blob/master/test/bash-preexec.bats#L198-L212

If users are still using trap they would come across other issues anyway, possibly not just #227 and #367. I think the right way to use trap is that there is only one thing in a Bash instance that owns the trap.

[FranklinYu]

For example, what if we have chpython, chnode, and chperl? Compare the hook solution

function chruby_hook() {
    chpython_auto
}

function chpython_hook() {
    chnode_auto
}

function chnode_hook() {
    chperl_auto
}

with the bash-preexec solution

preexec_functions+=(chruby_auto chpython_auto chnode_auto chperl_auto)

In addition, what about #399? Hence, my proposal: if preexec exist, use it (Bash or Zsh); fall back to trap (so that a fresh clean Bash “just works”).

[postmodern]

Then I’m once again advertising for https://github.com/rcaloras/bash-preexec,

How did I not see this sooner? bash-preexec looks like it would solve our problems and allow for better interoperability with other tools that wish to hook DEBUG. I am a little hesitant about adding a dependency that's not readily available via package managers. However, we could vendor bash-preexec and only load it when under bash?

[ilikepi]

Presumably you'd also check to see if bash-preexec was already loaded before attempting to load the vendored version?

Personally I'm not sure the added cost of vendoring a dependency is a net gain over the proposal in this PR, or the alternate proposal in #355 (comment). Incorporating bash-preexec seems counter to the minimalist nature of chruby. It could always be documented as a recommended addon for people who didn't want to mess with the DEBUG trap explicitly.

[FranklinYu]

I’m recommending detecting bash-preexec. I’m not suggesting vendoring it.

[ilikepi]

I’m recommending detecting bash-preexec. I’m not suggesting vendoring it.

Then to me, that change seems like it would be independent of this PR. Personally I'd prefer not to have to install a separate add-on in order to be able to more easily control when chruby_auto is called.