Do not use NotImplementedError

[hallelujah]

Instead use NoMethodError as to indicate that the method should be defined in the class

Closes #775

To do

• I have read the contributing guidelines.
• I have added relevant tests.
• I have adjusted relevant documentation.
• I have made sure the individual commits are meaningful.
• I have added relevant lines to the CHANGELOG.

PS: Thank you for contributing to Pundit ❤️

[Burgestrand]

Thank you for the PR! Please update the changelog.

[denisdefreyne]

I worry that NoMethodError will cause confusion, because the method does exist; it just isn’t properly implemented in subclasses.

In my own code, I’ve used my own SubclassResponsibilityError, like this:

class SubclassResponsibilityError < StandardError
  def initialize(klass, method_name)
    super("Subclasses of #{klass} must implement the ##{method_name} method.")
  end
end

Would this make sense? It would mean adding a custom exception to the generator, which might not be what we want.

In any case, I do think NoMethodError is an improvement over NotImplementedError, so this PR gets a thumbs up from me!

[Burgestrand]

@denisdefreyne I understand your worry, and it's a valid point. 😊

Taking that into consideration, and weighing that against adding a custom exception to the generator, together with the fact that the error message is descriptive, I think it's worth seeing how this flies in the wild.

It's generated code, so if this turns out to be confusing for people then 1) we can adjust it and 2) nobody needs to wait for a new release.

[Linuus]

What is a bit odd about this to me is that respond_to?(:resolve) #=> true but then raise NoMethodError when called.

https://ruby-doc.org/core-3.0.0/NoMethodError.html

Raised when a method is called on a receiver which doesn't have it defined and also fails to respond with method_missing.

I'm not saying NotImplementedError is perfect:
https://ruby-doc.org/core-3.0.0/NotImplementedError.html

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.

But maybe it maps closer to how it's used in this context?

[Burgestrand]

Thank you!

My thinking is that this error is for developers and not something you would expect to see runtime. We could have our own error (MethodNotImplemented = Class.new(Pundit::Error)) and raise that, but I don't think increasing our API surface area (another public name) is worth it for this.

Main benefit here is no longer inheriting from ScriptError, which an unimplemented resolve definitely is not.

[Burgestrand]

FYI @hallelujah I updated this branch with a rebase, because main ran into a build issue on jruby due to no fault of this PR.

[franzliedke]

Strictly speaking, this is a BR break. NotImplementedError is not a subclass of StandardError, whereas NoMethodError is. That means the exception will now be handled by a rescue statement (without specific exception class).

See the classic https://www.honeybadger.io/blog/understanding-the-ruby-exception-hierarchy/.

[Burgestrand]

This change affects a generator for the ApplicationPolicy, which is typically run once in the lifetime of an application. If you've set up Pundit in your app already, this won't affect you.

If you're setting up Pundit fresh in a new application, then yes this might raise a different error than what you're used to. If you make a habit of rescue StandardError, then yes you might catch this error too.

Regardless, you should be able to bundle update pundit just fine, and all your pre-existing code should still work.

[franzliedke]

Of course, totally missed that it's in a generator. 🤦🏼