Closed hallelujah closed 9 months ago
Thank you for the PR! Please update the changelog.
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!
@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.
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?
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.
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/.
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.
Of course, totally missed that it's in a generator. 🤦🏼
Instead use
NoMethodError
as to indicate that the method should be defined in the classCloses #775
To do
PS: Thank you for contributing to Pundit ❤️