search

ackama / rails-template

Application template for Rails 7 projects; preloaded with best practices for TDD, security, deployment, and developer productivity.
Other
294 stars 15 forks source link

fix: check for `NoMethodError` instead of `NotImplementedError` #545

Closed G-Rath closed 3 months ago

G-Rath commented 3 months ago

Pundit have recently changed to using NoMethodError, which breaks our spec - the main benefit of this change is that the new error extends from StandardError so in theory is easier to catch; however, it seems the community is divided as aside from the different superclass the existing error is more accurate since NoMethodError is meant to be thrown when the class does not respond_to? the method in question and that is not true.

I personally would like to keep using NotImplementedError though I'm happy to change if someone does prove a real-world situation where it's more burdson to catch (or that our error monitoring won't catch it), but I feel that should be a separate conversation - so this PR is changing it back to maintain our current status quo until such time that we do decide to change.

G-Rath commented 3 months ago

This is blocked by #546, which is blocked by this PR

robotdana commented 3 months ago

lets do this now, and revisit when some variant of https://bugs.ruby-lang.org/issues/18915 lands