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

restrict with-exit-codes to "run", "bash" and "system" #2742

Merged
merged 5 commits into from
Oct 15, 2019

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Oct 14, 2019

This PR follow a remark by @aalekseyev that the current semantics of with-exit-codes are not quite OK: in (with-exit-codes 2 (progn (exit 2) do_thing), do_thing will run, which is not expected.

Some alternatives were discussed:

  1. fix the behaviour: this means propagating the exit code of each run so that the enclosing action can decide whether to continue sequentially or simply return until the nearest with-exit-codes handler is found.

  2. switch to a special action (with-accepted-exit-codes <codes> <prog> <args>)

  3. add an optional argument to run to specify accepted exit codes.

I investigated 1), but the change seemed to complicate the code throughout simply to support this feature. It wasn't clear to me that the tradeoff was worth it. On the other hand, extending the syntax as in 3) is a bit tricky and needs more discussion.

So I implement 2) in this PR.

@aalekseyev
Copy link
Collaborator

A fourth plausible alternative is to keep the syntax as is, but make it an error to use with-accepted-exit-codes with anything except run, bash and system.

@nojb
Copy link
Collaborator Author

nojb commented Oct 14, 2019

A fourth plausible alternative is to keep the syntax as is, but make it an error to use with-accepted-exit-codes with anything except run, bash and system.

Good idea, this is a bit more flexible but still avoids the issue in question. I will wait a bit to see if there are more opinions, but this seems like a good compromise.

nojb added 4 commits October 14, 2019 21:39
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb force-pushed the run-with-accepted-exit-codes branch from 6206a72 to 5d51909 Compare October 14, 2019 19:41
@nojb nojb changed the title with-exit-codes => run-with-accepted-exit-codes restrict with-exit-codes to "run", "bash" and "system" Oct 14, 2019
@nojb
Copy link
Collaborator Author

nojb commented Oct 14, 2019

I reworked the PR along the suggestion of @aalekseyev (and renamed with-exit-codes => with-accepted-exit-codes). Review welcome !

Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nojb nojb merged commit 48d5aeb into ocaml:master Oct 15, 2019
@nojb nojb deleted the run-with-accepted-exit-codes branch October 15, 2019 12:28
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