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

feat Adding support for set -e and set -x #181

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

prsabahrami
Copy link
Contributor

Added support for set -e

@prsabahrami prsabahrami linked an issue Nov 27, 2024 that may be closed by this pull request
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 42.55319% with 27 lines in your changes missing coverage. Please review.

Project coverage is 62.23%. Comparing base (cc84630) to head (f90b8ed).

Files with missing lines Patch % Lines
crates/shell/src/commands/set.rs 0.00% 16 Missing ⚠️
crates/deno_task_shell/src/shell/commands/args.rs 33.33% 6 Missing ⚠️
crates/deno_task_shell/src/shell/types.rs 50.00% 4 Missing ⚠️
crates/deno_task_shell/src/shell/commands/sleep.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
- Coverage   62.64%   62.23%   -0.42%     
==========================================
  Files          30       31       +1     
  Lines        3183     3217      +34     
==========================================
+ Hits         1994     2002       +8     
- Misses       1189     1215      +26     
Files with missing lines Coverage Δ
crates/deno_task_shell/src/shell/command.rs 34.75% <100.00%> (ø)
crates/deno_task_shell/src/shell/commands/cp_mv.rs 95.34% <ø> (+0.77%) ⬆️
crates/deno_task_shell/src/shell/commands/head.rs 84.72% <100.00%> (ø)
crates/deno_task_shell/src/shell/commands/mkdir.rs 97.05% <100.00%> (ø)
crates/deno_task_shell/src/shell/commands/mod.rs 97.61% <ø> (ø)
crates/deno_task_shell/src/shell/commands/pwd.rs 95.83% <100.00%> (ø)
crates/deno_task_shell/src/shell/commands/rm.rs 100.00% <100.00%> (ø)
crates/deno_task_shell/src/shell/commands/xargs.rs 56.73% <100.00%> (ø)
crates/deno_task_shell/src/shell/execute.rs 75.13% <100.00%> (+0.03%) ⬆️
crates/shell/src/commands/mod.rs 41.66% <100.00%> (+2.01%) ⬆️
... and 4 more

... and 1 file with indirect coverage changes

@certik
Copy link
Contributor

certik commented Nov 28, 2024

Excellent, thanks for working on this! We need this.

Let's figure out how to test it.

Also, I think the default should be "set -e" (exit on error), but you can turn it off with "set +e". That deviates from the default Bash behavior, however since it makes it more strict, I think it might be fine. It still leaves the door open to later relax it if we decide it should not be the default. Looking at all my scripts, I think I've always needed "set -e". @wolfv what do you think the default should be?

@prsabahrami
Copy link
Contributor Author

It would be quite simple to change default. Also, very fair point, I think having it enabled would be more useful.
I yet to test this behaves exactly as expected especially in if statements and add tests hence leaving it as a draft.
Let me know if you figured out a way to properly test this but as for start, we can add scripts for testing.

@certik
Copy link
Contributor

certik commented Nov 28, 2024

One way to test it is like this. With set +x:

# something that fails
# something that succeeds

I think in Bash the whole script should succeed (we should check). While with set -x the script should fail in Bash. So we can simply check the return value of the script and ensure it is 0 with set +x and non-zero with set -x.

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.

Support "set -ex"
2 participants