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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect use of Process::exit. This fixes open issue #244. #578

Merged
merged 1 commit into from Jun 29, 2018
Merged

Fix incorrect use of Process::exit. This fixes open issue #244. #578

merged 1 commit into from Jun 29, 2018

Conversation

jmax315
Copy link
Contributor

@jmax315 jmax315 commented Sep 19, 2017

馃寛
Unlike the C library function exit(), Ruby's version takes a boolean,
not an integer. Added a spec to check the actual shell-level exit
value of a Thor script, and fixed calls to Process::exit.

Unlike the C library function exit(), Ruby's version takes a boolean,
not an integer. Added a spec to check the actual shell-level exit
value of a Thor script, and fixed calls to Process::exit.
@jmax315
Copy link
Contributor Author

jmax315 commented Sep 19, 2017

The build for 1.8.7 fails with problems in the webmock gem. As far as I can see, my change didn't cause these; does the current head build for 1.8.7 in CI correctly?

@lostapathy
Copy link
Contributor

#583 fixes the travis error that shows up here.

@deivid-rodriguez
Copy link
Contributor

@rafaelfranca What do you think about this? Backwards compatibility considerations about changing the default for this setting apart, this is fixing an actual bug, right?

@deivid-rodriguez
Copy link
Contributor

Would it help if I rebase this PR and get it green? According to #244 (comment), even setting exit_on_failure? to true does not exit on failure. This PR seems like a feasible explanation and fix for that issue, and it's backwards compatible.

@deivid-rodriguez
Copy link
Contributor

@rafaelfranca Since you're having a look at thor I'd like to politely ping you about this again.

@rafaelfranca rafaelfranca merged commit 0ecea7b into rails:master Jun 29, 2018
@deivid-rodriguez
Copy link
Contributor

Thanks! 鉂わ笍

The tests need to be skipped on 1.8.7 due to using spawn (or find another version that works ok on 1.8.7). Skipping seems easiest, no?

@rafaelfranca
Copy link
Member

Yes. Skipping is fine

@deivid-rodriguez
Copy link
Contributor

Ok, I'll open a separate PR to get it green!

@deivid-rodriguez
Copy link
Contributor

Funny, while creating the PR to skip the tests on 1.8.7 I tried reverting the code changes in this PR and the specs still passed. So it seems that this PR didn't fix anything and the exit_on_failure? thing is actually working fine? The tests still seem good to catch future regressions so I guess it's ok.

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