NotImplementedError should be prefered for abstract definitions

[cenouro]

This suggestion concerns this. The style guide gives good reasons to use fail, but I think that for abstract interfaces raise NotImplementedErrorshould be preferred way and mentioned on the style guide as well. This small discussion should be somewhat helpful also.

[besen]

Wouldn't it be weird giving style suggestions about abstract methods when the same guide says to avoid them?

[cenouro]

I agree that duck typing should be preferred, but at the same time, that will not always be the case. We may want to enforce that a class implements the full interface/contract; and I don't think we can simply banish inheritance in favor of duck typing and/or composition. Also, even though I usually prefer duck typing, sometimes inheritance captures and expresses better the semantics of the application.

Having said that, I think the guide should at least point that NotImplementedError is an acceptable exception to style guideline.

[fishermand46]

Why should raise be preferred to fail? They do the same thing; why should an abstract method be an exceptional case?

[bbatsov]

Yep, I agree with @fishermand46. There's nothing special here - just a failure, we've obviously envisioned.

[cenouro]

@fishermand46 and @bbatsov , you are correct. I failed to read further this section, from which follows that fail NotImplementedError complies with the style guide.

I still suggest being a bit more verbose on the guide and putting an example of abstract methods using fail to avoid some misuse of the guide, like happened in this gem. We can see they have not a standard, for they use:

• fail 'not implemented'
• fail 'not implemented, abstract'
• fail 'not impl.' # digging deeper in some files

I understand the way they did it is not your responsibility; still, I think that is a place where the guide should give a standard way of doing things, for example:

• fail NotImplementedError, 'abstract'
• fail NotImplementedError, 'todo'

[crudson]

Note that NotImplementedError is supposed to be "Raised when a feature is not implemented on the current platform. For example, methods depending on the fsync or fork system calls may raise this exception if the underlying operating system or Ruby runtime does not support them."

It isn't equivalent to, say, UnsupportedOperationException in Java.

[bbatsov]

I'm fine with adding more examples. PRs welcome! :-)

[cenouro]

@crudson that is an interesting point, but I believe that the majority of Ruby code runs on Linux/Windows/OSX.

@bbatsov thanks man.

[pirj]

NotImplementedError:

Raised when a feature is not implemented on the current platform. For example, methods depending on the fsync or fork system calls may raise this

NoMethodError:

Raised when a method is called on a receiver which doesn't have it defined

Are there any downsides to using NoMethodError?

[romankovt]

The main problem with NotImplementedError besides the initial purpose of the exception stated in the docs is that it is not a subclass of a StandardError but a ScriptingError, and all of the code which relies on rescue StandardError wouldn't work (see Parent section in the docs https://ruby-doc.org/core-3.0.0/NotImplementedError.html)

[pirj]

I've grepped through real-world-rspec for (raise|fail) NoMethodError and (raise|fail) NotImplementedError, and there are approximately 861 usages of NotImplementedError versus 23 usages of NoMethodError.

I encourage someone with more free disk space to grep through real-world-ruby-apps for a better sample, but I'd guess that the ratio will remain the same.

Being inclined to the "right" usage, which logically NoMethodError is, I'd personally rather not add a guideline. But the vast majority of usages of NotImplementedError suggests that it should be preferred, for consistency.

The argument that NotImplementedError does not inherit from StandardError doesn't seem to be on the side of using NoMethodError. On the contrary, since it's an error that can't be reasonably worked around in the code, it only makes sense to log it, or report it to the error tracking software. And those tools will do the job, and would let the application fail, since it's not a business error, but rather an unrecoverable programmatic one.