Raise NotImplementedError in attr_implement

[monkbroc]

I have another small improvement suggestion: if attr_implement raises NotImplementedError instead of RuntimeError, the error will pass through a rescue without a specific class (since that rescues RuntimeError).

This way a coding mistake will be found earlier if a class with attr_implement is used within a rescue block. Plus the name of the error is more descriptive.

NotImplementedError is a subclass of ScriptError instead of StandardError (see this handy Ruby exception cheat sheet)

[henrik]

I'm happy to change it to a named exception class other than RuntimeError.

I wonder if NotImplementedError is a good choice, though. It's documented as

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.

Also, my impression of Ruby best practise has been to inherit most exceptions from StandardError (so a blanket rescue catches it). The ones in the cheat sheet that are outside StandardError all seem more foundational/serious than this.

If you call "foo".no_such_method, it raises NoMethodError. That seems like a good choice. Inherits from StandardError. What do you think?

[monkbroc]

I think StandardError indicates a (potentially) occasional computation issue like sending a message to an object that doesn't understand it (NoMethodError).

In the attr_implement case, it's a true coding error. The programmer forgot to write part of the code. Making the error rescuable with a blanket rescue would mask it longer I would say.

I think it's justified to have an error outside of StandardError in this case.

Avdi Grimm, proficient Rubyist and author of the Ruby Tapas screencast (nice video series by the way) recommends using NotImplementedError for methods in abstract classes in one Ruby Tapas episode:

def condition_holds?
  raise NotImplementedError, 
        "You must implement a #condition_holds? predicate that " \
        "tests whether the condition is currently true"
end

[henrik]

Hm, you make a good argument.

Does @tskogberg, @joakimk or @soma have an opinion either way?

[joakimk]

I don't like using NotImplementedError because it has another meaning in standard ruby.

Maybe we could define our own exception that inherits from Exception instead of StandardError. Something like MethodNotImplementedError?

[henrik]

That sounds fair. There's no value in using the pre-existing exception, really, that I can see. I love Avdi's work but he has some wonky ideas like anyone else ;)

[monkbroc]

Funny comment @henrik :)

I'm not attached to the NotImplementedError name, especially since it already means something else in Ruby core.

[henrik]

:) Alright, so if you change it to a MethodNotImplementedError that inherits from Exception, I'm happy to merge.

I'd also remove the "it "raises an exception that is not caught by a generic runtime error rescue" do test – that seems like testing the behavior of rescue more than it tests our code. I'd just put a comment next to where we define the exception and explain briefly why we don't want a StandardError.

[monkbroc]

OK. That's done.

[henrik]

Awesome, thank you!

[henrik]

Released as 4.3.0.

[teoljungberg]

@joakimk why did you want to move away from NotImplementedError? It's the convention to raise that in this case as far as I've seen

[joakimk]

@teoljungberg as you can see above, it's about NotImplementedError having another meaning in ruby core. Their docs: 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..

In other words, for features that some platforms do not have, not for defining subclass APIs.

[henrik]

@teoljungberg I think the value in using that specific error is probably low. We've changed from RuntimeError which is generic and can be implicitly rescued to something else that is 1. less generic and 2. is not implicitly rescued. So those are clear wins.

In my eyes, any "convention" that exists (e.g. Avdi proponing it as discussed above) is probably more a case of people misguidedly taking the exception name without context and not thinking enough about what the built-in error actually is used for in Ruby itself.

Reusing the name probably doesn't add much value, but it adds a slight risk that someone who intends to rescue NotImplementedError to say "Sorry, you need to use Linux" or whatever would incorrectly rescue API mistakes.