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

Abort if the command for run fails and exit_on_failure? is true #651

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

hibariya
Copy link
Contributor

Hi, there 🌈

I'd like to change the error handling of Thor::Actions#run for the following reason.

According to the description of exit_on_failure?, the process should exit with status 1 if any error happens. I think that it should be also applied to run.

Although we can pass abort_on_failure option for run, passing it every time seems a little redundant when we call run multiple times. This is the other reason for this change.

For instance, when an error occres here, I want to notice not only by the output but also by the status code.

@hibariya
Copy link
Contributor Author

#649 is needed to fix the error on CI. A build with that patch seems work.
https://travis-ci.org/hibariya/thor/builds/505774428

@deivid-rodriguez
Copy link
Contributor

I really like this idea! ❤️

Even after adding the abort_on_failure: flag I still miss errors in my generators, because I cannot control every call to Thor::Actions#run.

For example, I just noticed that our Rails 6 application generation in ActiveAdmin is crashing, but the crash is only buried in the logs, so there's no way to notice other than manual log scanning (or if the failure affects further tests, but in that case, the culprit is still very hidden).

According to the description of `exit_on_failure?`,
the process should exit with status 1 if any error happens.
I think that it should be also applied to `run`.

Although we can pass `abort_on_failure` option for `run`,
passing it every time seems a little redundant when we call
`run` multiple times. This is the other reason for this change.
@hibariya
Copy link
Contributor Author

I've rebased onto the top of the master. The build seems OK.

@deivid-rodriguez
Copy link
Contributor

Any chance on getting a review here?

@kaspth
Copy link

kaspth commented Nov 10, 2019

👍, this also matches the original intent #628

I don't have commit access, so it'll have to be @rafaelfranca who merges ❤️

rafaelfranca added a commit that referenced this pull request Nov 16, 2019
Abort if the command for `run` fails and `exit_on_failure?` is true
@rafaelfranca rafaelfranca merged commit 5738a3b into rails:master Nov 16, 2019
unasuke added a commit to itamae-kitchen/itamae that referenced this pull request Dec 31, 2021
unasuke added a commit to itamae-kitchen/itamae that referenced this pull request Dec 31, 2021
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.

None yet

4 participants