-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix filters being called multiple times, consistent intialization for… #1465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix filters being called multiple times, consistent intialization for… #1465
Conversation
5e7fa0b
to
c4e4f07
Compare
@@ -248,6 +248,8 @@ def run | |||
|
|||
run_filters befores, :before | |||
|
|||
raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]) if env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you get rid of the duplication of env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. It only is duplicated if it is present so I thought it was okay. Do you have a preference between these two?
1:
if (allowed_methods = env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED])
raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods)
end
2:
allowed_methods = env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]
raise Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allowed_methods) if allowed_methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 1 will be aborted by rubocop, so I'd like to vote to 2 approach.
Btw, there are two reasons why I have pointed out.
- By assiging variable, we can reduce number of characters of the line.
- The assigned variable can be used in three places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubocop was fine with both(the parens around it quieted it). But it caused the class to have too many lines. option 2 seems to take care of that restraint, but either that limit will need to be raised soon or....
and the 3rd instance of env[Grape::Env::GRAPE_METHOD_NOT_ALLOWED]
is actually completely redundant since we are raising an exception. So I removed it with my latest change.
@jsteinberg Awesome. Your patch seems to work well. |
… method_not_found errors
c4e4f07
to
2d8ef47
Compare
Excellent, merging. |
… method_not_found errors
I also think this is a more consistent fix to this: #1446