from @DannyBen here

I believe the trap can and should be in the initialize. Do you see any reason against it?

Nothing against it! 👍

Is there any case where this error trap is executed during a "normal" bashly behavior? For example, when insufficient arguments were provided by the user?

I didn't face any situation of the trap being executed by a "normal" bashly behavior.

Specifically about argument handling, I've tested here and all situations were smoothly handled by bashly's parse_requirements function.

To do the tests I installed the stacktrace in the examples/validations and executed some wrong commands.

$ tree -F -L 3
./
├── README.md
├── src/
│   ├── bashly.yml
│   ├── build_command.sh
│   ├── calc_command.sh
│   ├── deploy_command.sh
│   ├── initialize.sh 👈
│   └── lib/
│       ├── stacktrace.sh 👈
│       └── validations/
├── test.sh
└── validate*

$ ./validate invalid_command
invalid command: invalid_command

$ ./validate calc
missing required argument: FIRST
usage: validate calc FIRST [SECOND] [OPTIONS]

$ ./validate calc one two
validation error in FIRST:
must be an integer

$ ./validate calc 1 two
validation error in SECOND:
must be an integer

$ ./validate calc 1 2 --save
--save requires an argument: --save PATH

$ ./validate calc 1 2 --save invalid_file
validation error in --save PATH:
must be an existing file

$ ./validate build
validation error in environment variable BUILD_DIR:
must be an existing directory

$ ./validate build invalid_dir
validation error in environment variable BUILD_DIR:
must be an existing directory

$ ./validate deploy
missing required flag: --user USERNAME

$ ./validate deploy --user
--user requires an argument: --user USERNAME

$ # 👇 here the stacktrace should trigger 👇

$ cat src/deploy_command.sh
echo "# this should trigger the stacktrace"
no_such_command

$ ./validate deploy --user meleu
# this should trigger the stacktrace
./validate: line 283: no_such_command: command not found
./validate:283 in `validate_deploy_command`: no_such_command

Stack trace:
        from ./validate:283 in `validate_deploy_command`
        from ./validate:621 in `run`
        from ./validate:627 in `main`

@pcrockett do you wish to review this before I merge?

Oh, might consider mentioning the strict setting as an alternative to adding set to initialize.sh.

Oh, might consider mentioning the strict setting as an alternative to editing initialize.sh.

I thought about it, but decided to separate - since you still have to add the trap command anyways.

I've just noticed something while using BATS for unit testing here. No actions to be taken on bashly side, I'm just sharing a recent finding

TL;DR

Nothing breaks, just the failing test output becomes more "polluted" when the script is sourced.

Solution: disable the trap right after sourcing the script (trap '' ERR)

long version

I usually do unit testing by sourcing the final script and then in each test I call the individual function under test.

I noticed that with stacktrace enabled, the test failure output becomes polluted.

examples

Here are two examples adapted from a real test (ignore the test logic and focus on the output).

keeping the ERR trap

Test snippet:

@test "when value is a file with placeholders, they are replaced (keep trap)" {
  source "${BASHLY_TARGET_DIR}/my_app" # 👈 sourcing here

  value_file="${TEST_TMP_DIR}/full_name.txt"
  echo "John {{surname}}" > "$value_file"
  echo "Hello, {{name}}!" > "$template_file"

  run template_apply "$template_file" "name=$(< "$value_file")" #surname="Doe" # 👈 force failure

  assert_output "Hello, John Doe!"
}

Polluted failure output:

$ bats -f 'keep trap' test/lib/simple_templating_test.bats
simple_templating_test.bats
 ✗ when value is a file with placeholders, they are replaced (keep trap)
   (from function `stacktrace' in file test/lib/my_app, line 990,
    from function `__assert_stream' in file test/lib/../test_helper/bats-assert/src/assert_output.bash, line 246,
    from function `assert_output' in file test/lib/../test_helper/bats-assert/src/assert_output.bash, line 125,
    in test file test/lib/simple_templating_test.bats, line 98)
     `assert_output "Hello, John Doe!"' failed

   -- output differs --
   expected : Hello, John Doe!
   actual   : Hello, John {{surname}}!
   --

   /path/to/project/test/lib/my_app:246 in `__assert_stream`: fail
   Stack trace:
        from /path/to/project/test/lib/../test_helper/bats-assert/src/assert_output.bash:246 in `__assert_stream`
        from /path/to/project/test/lib/../test_helper/bats-assert/src/assert_output.bash:125 in `assert_output`
        from /path/to/project/test/lib/simple_templating_test.bats:98 in `test_when_value_is_a_file_with_placeholders-2c_they_are_replaced_-28keep_trap-29`
        from /home/linuxbrew/.linuxbrew/Cellar/bats-core/1.11.1/libexec/bats-core/bats-exec-test:355 in `bats_perform_test`
        from /home/linuxbrew/.linuxbrew/Cellar/bats-core/1.11.1/libexec/bats-core/bats-exec-test:377 in `main`

1 test, 1 failure

resetting the ERR trap

Test snippet:

@test "when value is a file with placeholders, they are replaced (reset trap)" {
  source "${BASHLY_TARGET_DIR}/my_app"
  trap '' ERR  # 👈 reset the ERR trap

  value_file="${TEST_TMP_DIR}/full_name.txt"
  echo "John {{surname}}" > "$value_file"
  echo "Hello, {{name}}!" > "$template_file"

  run template_apply "$template_file" "name=$(< "$value_file")" #surname="Doe"

  assert_output "Hello, John Doe!"
}

Cleaner failure output

$ bats -f 'reset trap' test/lib/simple_templating_test.bats
simple_templating_test.bats
 ✗ when value is a file with placeholders, they are replaced (reset trap)
   (from function `__assert_stream' in file test/lib/../test_helper/bats-assert/src/assert_output.bash, line 246,
    from function `assert_output' in file test/lib/../test_helper/bats-assert/src/assert_output.bash, line 125,
    in test file test/lib/simple_templating_test.bats, line 111)
     `assert_output "Hello, John Doe!"' failed

   -- output differs --
   expected : Hello, John Doe!
   actual   : Hello, John {{surname}}!
   --


1 test, 1 failure

I could have guessed that trapping error might cause issues in some cases.
I think this is up to the developer to do a conditional trap, something like "if STACKTRACE variable is set" or similar.

I think I'll stick with this approach for now and see if it works fine:

Trap ERR only if the script is NOT being sourced.

# initialize.sh

if [[ "${BASH_SOURCE[0]}" != "${0}" ]]; then
  trap 'stacktrace' ERR
fi

I think I'll stick with this approach for now and see if it works fine

Yup. Anything that works for your use case and audience is valid.

@DannyBen one thing that just came to my mind:

The official stacktrace.sh could have this:

enable_stacktrace() {
  trap 'stacktrace' ERR
  set -o errtrace
}

That would be similar to what we did with enable_auto_colors.

I this allows the src/initialize.sh to look a bit tidier. A typical one for my bashly projects would look like this:

# src/initialize.sh
enable_auto_colors

is_sourced || enable_stacktrace

# whatever...

What do you think?

Also, a post-install hook could give instructions similar to what we do for colors:

@DannyBen one thing that just came to my mind:

Lovely. One thing that bothers me, is the bashly add hooks since it adds all hooks, and two of them are "noisy" by default, and have to be deleted by the user. I was toying with the idea of allowing bashly add hooks initialize or bashly add initialize. to mitigate this and still remain backwards compatible.

Thoughts?

I was toying with the idea of allowing bashly add hooks initialize or bashly add initialize.

The first option makes more sense to me. As we are explicitly adding a hook named initialize.

The second option gives an impression that I'm adding a library named initialize.

Still about refining the stacktrace...

It's quite usual for bash coders to run bash -x script for debugging. Even with a stacktrace it's still useful to debug with -x.

The thing is that the xtrace's output for stacktrace is quite noisy and useless for debugging.

This is how I solved it:

stacktrace() {
  local exit_status="$?"
  set +x # 👈 MUST be after exit_status assignment
  # ...
}

It's important to disable xtrace after saving the previous exit status, otherwise it would get the exit status of the set command.

Do you think it makes sense to make this "official"?

One thing I considered: as the stacktrace function involves already has a hard exit at the end, I think that "arbitrarily disabling a user-defined bash option" wouldn't be surprising/confusing to the user. On the other hand, the noisy output is quite confusing.

It's quite usual for bash coders to run bash -x script for debugging. Even with a stacktrace it's still useful to debug with -x.

This seems like an over-intervention to me.
I for one, never do that, and I would imagine that if I need to use -x, I would want to disable stacktrace.

I feel this falls into the "personal habit" category.

Don't forget that all libraries are dumped into userland for this reason exactly - they serve as a good basis for you to edit if you need to. I do not feel that we need to anticipate the use of -x, and even if we do, it is not up to us to override and disable it.