Cop for raise/fail arguments is too restrictive.

[famished-tiger]

For an expression like:
fail(MyExceptionClass.new(some_exception_arg1,some_exception_arg1))
Where:

• MyExceptionClass is a custom exception class.
• some_exception_argX are arguments of that exception class constructor.

Rubocop will report an offence like:
... C: Provide an exception class and message as arguments to fail.
fail(UnreachableSubstepArgument.new(substep_arg))

This is problematic because:
-When one raises an Exception it is a good programming practice to provide as much context data as needed for a proper understanding. Restricting the calls to fail/raise to an exception class plus a message argument limits the kind of feedback you can provide upon raising an exception.
-Furthermore, calling fail/raise with an exception object is perfectly legal Ruby:
"if raise is called with a single Exception object as its argument, it raises that exception". Sentence taken from: page 157 of "The Ruby Programming Language" of D. Flanagan & Yukihiro Matsumoto, O'Reilly 2008 ISBN 978-0-596-51617-8.

[bbatsov]

The question is not whether this is legal Ruby code or not - obviously it is. The point is to enforce either fail Exception, msg or fail Exception.new(msg). The cop is configurable, and you can always disable it if you disagree with the two acceptable coding styles. Personally, I've written a lot of Ruby code and never defined an exception class with a constructor taking anything other than a message, since that's not consistent with the exception classes in the standard library.

[famished-tiger]

Well the point is whether one allows the following expression is acceptable in Rubocop:
fail MyException.new(arg1, arg2,...)
Where: arg1, arg2 are not (necessarily) the error message. This idiom is particularly useful:
-When an exception handler tries a fallback alternative and uses the passed arg1, arg2 objects; or
-When the 'final' error message is generated in another place than the method raising the exception. There are very reasons to do this: for instance, the error message must be internationalized/localized or the format of the error message must depend on the output stream (logger, screen; file, ...) and/or the desired output format (plain text, HTML file, JSON, ...). We shouldn't spread these decisions over all sites where an exception might be raised. In fact, in a multi-lingual application, enforcing calls like fail MyException, msg will be a serious obstacle.

It's true that by far exception constructors take a -part of- message as the sole argument. But that's not always true. Let's take an example from the standard library, namely rubygems. It defines the initialize method for its exception class Gem::GemNotFoundException as follows:
class Gem::GemNotFoundException < Gem::Exception def initialize(msg, name=nil, version=nil, errors=nil) super msg @name = name @version = version @errors = errors end

Well as it now rubocop will flag this as an offense while this is perfectly sensible code.

[bbatsov]

Fair enough. I'll relax the requirements and allow the use of Exception.new(...) with more than 1 argument.

[famished-tiger]

Blagodaria. :)

[bbatsov]

За нищо! :-)

[segiddins]

@bbatsov shouldn't the use of Exception.new with one argument still be allowed if that argument is not a string?

[famished-tiger]

@segiddins Good remark. In principle, when designing exception constructors you should be completely be free to define their signature (number of arguments and type of arguments). On the other hand, if the current cop implementation satisfies 95% of the cases maybe one could just disable it when it shouts when it shouldn't. Maybe it is not worth to change the cop for less frequent cases. Now I am not aware of stats about the ways exceptions are raised in some examplar projects/standard libraries. This could help in figuring out how frequent are the cases you mention occurring in "popular" Ruby code.

[AlexCppns]

I don't understand why rubocop finds an offense there:

Provide an exception class and message as arguments to raise.
    raise ActionController::RoutingError.new('Not Found')

[jonas054]

@AlexCppns It wants it to be

raise ActionController::RoutingError, 'Not Found'

This is the exploded style. Your example would be allowed with the setting

Style/RaiseArgs:
  EnforcedStyle: compact

[mattdowdell]

@bbatsov A bit of an edge case, but rubocop finds an offence with something like the following as well:

args = [message, other_arg_1, other_arg_2]
raise MyError.new(*args)

[jonas054]

@mattdowdell An edge case, yes. But still a bug since raise MyError, *args doesn't work if args has multiple elements. Can you open a separate issue for it?

[mattdowdell]

@jonas054 Opened at #3577. I'm not entirely sure what the expected behavior of raise MyError, *args should be as I'm relatively new to ruby. Would you be able to clarify what you meant in the new issue?