Define Pundit::Error using compact style. #590
Conversation
|
Yes! |
|
I mean the same can be said for the other errors, even though name clashes wouldn't be as common. Why does it make sense to mixin an |
@jensljungblad Yeah, I agree. The reason I only did the |
|
Right, you mean if some people catch Well, I'm for doing it in the next major release then :) |
Exactly :) My thought was doing this now and then fix everything in the next major release. I have some bigger structural changes I want to do anyway :) |
|
Nice! This is much better. |
|
@Linuus just to check that I understand correctly: it sounds like this one can be merged and released now, and it would be nice to do change the other errors as well, but that will require a major version update? |
|
IMO yes. |
When defining the class with: ``` module Pundit class Error end end ``` the `Error` class is mixed in to the controller when using `include Pundit`. This can cause issues since `Error` is a very common class name. Using the compact style: ``` class Pundit::Error ``` we avoid this, since it only creates the constant `Pundit::Error`.
dgmstuart
force-pushed
the
avoid-error-name-clash
branch
from
09893b0 to
b7b34d9
Compare
|
Rebasing... |
| # @api public | ||
| module Pundit | ||
| SUFFIX = "Policy".freeze | ||
|
|
||
| # @api private | ||
| module Generators; end | ||
|
|
||
| # @api private | ||
| class Error < StandardError; end | ||
|
|
||
| # Error that will be raised when authorization has failed | ||
| class NotAuthorizedError < Error |
There was a problem hiding this comment.
shouldn't this then be < ::Pundit::Error ?
There was a problem hiding this comment.
@bf4 Thanks for the comments - can you expand on this?
This does seem to inherit from the correct Error class if that's the concern:
pry(main)> require 'pundit'
=> true
pry(main)> Pundit::NotAuthorizedError.ancestors
=> [Pundit::NotAuthorizedError,
Pundit::Error,
StandardError,
Exception,
Object,
JSON::Ext::Generator::GeneratorMethods::Object,
PP::ObjectMixin,
Kernel,
BasicObject]
When defining the class with:
the
Errorclass is mixed in to the controller when usinginclude Pundit. This can cause issues sinceErroris avery common class name. Using the compact style:
we avoid this, since it only creates the constant
Pundit::Error.Solves #542
Closes #555 #570