Skip to content
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

Allow bash-preexec to be included at any point #110

Merged
merged 5 commits into from
Aug 22, 2020
Merged

Conversation

rcaloras
Copy link
Owner

- Relaxes requirement that bash-prexec.sh needs to be included as last
thing in bash_profile.sh
- Preserves existing prompt command in PROMPT_COMMAND variable. May make
sense to still move this to a function in precmd.
- Should address #97 and #39
- Keeps existing traps preserved as preexec functions
- Removes trailing \n on PROMPT_COMMAND added for #58.
- Added tests for adding before and after install in profile
- Changed install string to not lead with new line
@rcaloras
Copy link
Owner Author

rcaloras commented Aug 21, 2020

Doing some manual testing and added some unit tests and believe this should be good to go. cc @dimo414, @brandonweeks, @jombooth as relevant reviewers if anyone is free to take a look.

One potentially breaking aspect of this is:

Upon further inspection, I think appending a terminating character (e.g. ;, or \n) to the end of PROMPT_COMMAND is debatable. We actually chose against it in #41, and surveying a few other projects manipulating prompt command, it looks the standard is more towards adding terminating characters to the root when appending (i.e. PROMPT_COMMAND=$PROMPT_COMMAND\n new_function_being_appended).

- Need to update unit tests for echos on both cases
- Small edge case where preexec is not invoked the first time if
PROMPT_COMMAND is appended to after bash-preexec is sourced.
- Handle cases where we append with semicolon and \n
@dimo414
Copy link
Collaborator

dimo414 commented Sep 16, 2020

Apologies for not taking a look earlier. The PR looks great!

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