Returns exit 0 when a required argument is not provided

[msaffitz]

Simple example

#!/usr/bin/env ruby
require 'thor'

class Foo < Thor

  desc "foo", ""
  method_option :output, required: true
  def foo
    puts options[:output]
  end

end

Foo.start

$ ./foo.rb foo -output=blah                                                                                                                               [1s] 
No value provided for required options '--output'
$ echo $?                                                                                                                                                 [0s] 
0

I'd expect this to return an error exit code

[barttenbrinke]

This seems to be by design in thor - base.rb:

      rescue Thor::Error => e
        ENV["THOR_DEBUG"] == "1" ? (raise e) : config[:shell].error(e.message)
        exit(1) if exit_on_failure?

And futher down:

        def exit_on_failure?
          false
        end

[nbibler]

I believe this is also biting me. I'm using Thor in some Capistrano deployment scripts and when they fail, Capistrano is not exiting due to the 0 exit code... Even if this is by design, it should be documented somewhere and made clear the steps to produce an exit(1) event.

[nbibler]

I guess the current, correct approach is similar to:

class MyStuff < Thor
  def self.exit_on_failure?
    true
  end

  desc "epic_failure", "failure is epic"
  def epic_failure
    raise Thor::Error, "E-E-E-EPIC-C-C-c-c-c-c-.-.-."
  end
end

$ thor my_stuff:epic_failure
E-E-E-EPIC-C-C-c-c-c-c-.-.-.
$ echo $?
1

I think it just needs to be more clearly documented somewhere...

[leehambley]

I raised a similar issue directly with Bundler, who've referred me here. It's as simple as if you can't do what I ask, don't exit 0 – anything else is a bastardisation of 30 years of software sane practice.

rubygems/bundler#1892 (comment)

[b-dean]

So I know this issue was closed 5 months ago but I just wanted to mention that as a person writing command line applications it seems really weird to me that the default behavior is to exit with a 0 exit status on errors. That is so backwards.

I think the exit_on_failure? method is fine but I think it should return true by default. In fact, we monkey patch Thor::Base::ClassMethods so exit_on_failure? returns true for every CLI we make. That way if some weird corner case needs to raise an error and not exit with an error status it can have exit_on_failure? return true, but it works like normal CLI applications for every other case.

[ab]

This was very surprising to me as well. Is there some case where exiting with status 0 on errors is desirable? It doesn't seem like a good default.

Thanks b-dean for the monkey patch suggestion.

[mikz]

Maybe because of testing code? Do you test the apps, right? :)
Any error in thor will turn off your tests.

[ab]

It seems to me that you should explicitly opt in to that behavior if you really want it in testing. Personally I would test the library functionality in my app separately from the CLI. And if you want to test the CLI, you should probably be using a subprocess. Or use ThorClass#invoke directly.

[b-dean]

@mikz, It's not just a matter of whether or not we test our CLI apps. If the CLI app talks to some external system and something unexpected happens (or even a known problem) and the CLI app throws an exception, it does not make any sense to me why the default for Thor is to have this exit with a success status. Something bad happened, it should give an appropriate exit status otherwise it's a bad CLI app. Who knows the exception could even be some rare bug in Ruby itself, or some operating system level error (permissions, out of memory, etc). Those also shouldn't be 0 exit statuses (IMHO).

[mikz]

Definitely CLI apps should respond with non success error codes on failures. No dispute over that.

Thor allows you to set the exit_on_failure? in the binary and keep the .rb files raising exceptions for testing.

Thats the best of the both worlds, binary exits > 0 on errors and testing code keeps raising exceptions.

[b-dean]

I'm having a hard time understanding how having the default for exit_on_failure? being true would mess up your tests. You had said "Any error in thor will turn off your tests." but I don't really know what you mean by that. If you're testing with cucumber (maybe with aruba) and running the CLI or running thor commands, you could check that the exit code is non zero when you want to test for exceptions. If you're using rspec, you could have expect{thing.invoke(args)}.to raise_error SystemExit Or better yet, use cucumber/aruba to test the behavior of the CLI by running the CLI, and use rspec to test classes used by the CLI (that are normal ruby classes, not subclasses of Thor) and deal with the exceptions how you normally would.

My main point, is that a library that whose expressed purpose is for writing CLI applications (see the first sentence on http://whatisthor.com/), it is very strange for the default behavior (swallowing errors and always returning 0) to be breaking with the convention set by all CLI applications since the beginning of time (non-zero exit codes for failure).

[mikz]

Because aruba & cucumber are for integration testing.
If you have unexpected error in your unit tests and Thor would exit, your tests would quit too.

We agree that there should be a switch to turn it on/off. Something like when you run it with .start it will exit (because thats what you do in bin), but when instantiating them normally, it would not quit.

[leehambley]

Because aruba & cucumber are for integration testing.
If you have unexpected error in your unit tests and Thor would exit, your tests would quit too.

That rather implies that Aruba is broken, since testing failure conditions is arguably one of the more important sides of testing.

For what it's worth in pretty much every long lived POSIX compliant program, the use of specific error codes, or code ranges (not unlike HTTP) is a critical part of reliable scripting.

Thanks for making your case for Thor's (provably broken) behaviour. Correct error handling shouldn't be an option.

[b-dean]

@leehambley, I don't think @mikz was saying cucumber/aruba will quit when thor has an error. He was saying that "unit testing" would quit, where unit testing is something like rspec, test unit, or any number of other unit test frameworks. Cucumber is specifically not a unit test framework. Also using aruba and cucumber, you absolutely can test for failure conditions:

When I run `my_cli with some bad args`
Then the exit status should be 42

The reason I proposed cucumber/aruba for this problem is that I believe when you are testing the CLI, you are doing integration testing because the user will be using the CLI as their user interface. That's why cucumber exists. If you use cucumber for testing the CLI, you can still use rspec or whatever else to test the underlying classes that your CLI uses.

But maybe your CLI class is huge and all the logic for your program is right inside your Thor subclass. (That could be tricky to test anyway but nevermind that). If you insist on using unit testing frameworks to test your Thor subclasses, you could always use a mock framework to change what exit_on_failure? returns:

Before(:each) do
  MyCli.stub(:exit_on_failure?).and_return(false)
end

Then it won't exit on failure. But you can also do what I mentioned earlier and have your tests expect a SystemExit error when you were expecting your cli to raise some exception.

My point is: there are a lot of options for solving the testing problems, but having a framework whose whole purpose is for quickly and easily making CLI applications have the default behavior of exiting with 0 when there are errors is horrible.

[leehambley]

My point is: there are a lot of options for solving the testing problems, but having a framework whose whole purpose is for quickly and easily making CLI applications have the default behavior of exiting with 0 when there are errors is horrible.

Right, and the Thor maintainers(s) don't seem interested in fixing that, or even accepting that 30 years of computer history tells them that's broken.

For what it's worth, it's pretty easy to subclass Rake to build a small CLI application.

[dserodio]

That's a rather surprising default indeed

[chiefy]

Just my opinion, but exiting 0 when you fail to enter the proper args is just plain ludicrous. I know you can change it, and I have, but... just no.

Thor is pretty great otherwise, seems like something relatively easy to fix? I know it'd be a breaking change, but it would be for the benefit of sanity.

[wjbuys]

I traced through the history for exit_on_failure?, and it turns out this was "fixed" in the very first Thor issue (#1) but only for the rake-style usage. However, the detailed repro in #56 suggests that it's actually supposed to have been exiting with nonzero for all cases. How bad would it be to just change the default to true?

[longhotsummer]

+1 for non-zero exit code on failure. In my case, when you provide a command that it doesn't know about. Very strange not to have that by default.

[voondo]

+1 too. It is a very strange behavior for a CLI library...

[dkniffin]

Hmm. I'm very curious why this hasn't already been resolved. It seems like everyone is in agreement that the current default is incorrect. I've gone ahead and created a PR for this change (#521).

[barttenbrinke]

It would change the current behaviour of a lot of widespread Ruby tools, probably causing a lot of issues.

[dkniffin]

Ok, so we bump the major version of Thor, and put it in the changelog. That's what semantic versioning is for, right?

[EugenMayer]

i guess this will never get implemented, eventhough that is pretty critical?

The case #244 (comment) is still not working, even with adding

def self.exit_on_failure?
    true
  end

[perlun]

Ok, so we bump the major version of Thor, and put it in the changelog. That's what semantic versioning is for, right?

It is indeed. The problem with Thor is that it's never reached the 1.0 version, which is what SemVer defines as:

4. Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.
5. Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.

So, we have a version which is really defined for "initial development" even though it has been in widespread production usage for years. This is of course not optimal, but we must also remember and accept that when Thor was initially created, the SemVer specification as we know it today didn't even exist. Things are a lot different now than they used to be back then.

Anyhow, I do agree with your point: we should work towards a 1.0 release of Thor, so we can start applying reasonable SemVer semantics to the releases etc.

My suggestion (this is mostly directed to @wycats or someone else working with the maintenance of this gem):

• Bump the version of the current master from 0.19.4 to 1.0.0 straight away, and publish it as a Rubygem. Make it a public announcement that the public API is now to be trusted.
• Start working on the next major version, i.e. 2.0.0. In that version, small annoyances like this (which I definitely agree is a bad design decision, probably with historical reasons behind it) can be smoothed over.

Hand raised ✋: I am willing to volunteer in this as needed, but is not currently a contributor or maintainer of this gem. If help/PRs are requested to get this moving, please let me and others know and we'll do our best to contribute back to a very valuable Rubygem that means a lot to many of us.

[mohkale]

3 years since the last comment, 8 since this issue began, any update on this?

[deivid-rodriguez]

@mohkale Currently if you run the program in the initial post of this ticket, you get a useful deprecation warning:

./test.rb foo -output=blah
No value provided for required options '--output'
Deprecation warning: Thor exit with status 0 on errors. To keep this behavior, you must define `exit_on_failure?` in `Foo`
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.

So this should get fully fixed in the next major release (I guess), and you can already opt-in to the right behaviour very easily.

[janie314]

I guess the current, correct approach is similar to:

class MyStuff < Thor
  def self.exit_on_failure?
    true
  end

  desc "epic_failure", "failure is epic"
  def epic_failure
    raise Thor::Error, "E-E-E-EPIC-C-C-c-c-c-c-.-.-."
  end
end

$ thor my_stuff:epic_failure
E-E-E-EPIC-C-C-c-c-c-c-.-.-.
$ echo $?
1

I think it just needs to be more clearly documented somewhere...

Two thumbs up reacts and I'll make a PR to stick this in the repo's README.