Skip to content
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

add back message key in order to have error_codes #1366

Merged
merged 2 commits into from
Apr 22, 2016

Conversation

mkou
Copy link
Contributor

@mkou mkou commented Apr 19, 2016

In my situation, in the exception validations, I need message keys as well as the translations in order to pass error codes

@dblock
Copy link
Member

dblock commented Apr 20, 2016

This needs tests and a CHANGELOG entry, please.

@mkou
Copy link
Contributor Author

mkou commented Apr 20, 2016

@dblock Done. Thanks for the feedback

@@ -9,6 +9,7 @@ class Validation < Grape::Exceptions::Base
def initialize(args = {})
fail 'Params are missing:' unless args.key? :params
@params = args[:params]
@message_key = args[:message] if args.key?(:message) && args[:message].is_a?(Symbol)
args[:message] = translate_message(args[:message]) if args.key? :message
super
Copy link
Member

@dblock dblock Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I nitpick? Is this better? Avoids checking key? twice.

return unless args.key?(:message)
@message_key = args[:message] if args[:message].is_a?(Symbol)
args[:message] = translate_message(args[:message])

or with an if, I don't care.

Copy link
Contributor Author

@mkou mkou Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm wouldn't the return bypass the super method?
I like the idea of not checking the key twice though.

Copy link
Contributor Author

@mkou mkou Apr 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer that version ?

super && return unless args.key?(:message)
@message_key = args[:message] if args[:message].is_a?(Symbol)
args[:message] = translate_message(args[:message])
super

or

if args.key?(:message)
  @message_key = args[:message] if args[:message].is_a?(Symbol)
  args[:message] = translate_message(args[:message])
end
super

(I think I prefer the last one)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely the latter 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise I'm with @ridiculous

@mkou
Copy link
Contributor Author

mkou commented Apr 22, 2016

Thanks! 😄 What do you think?

@dblock dblock merged commit 59af167 into ruby-grape:master Apr 22, 2016
@dblock
Copy link
Member

dblock commented Apr 22, 2016

Merged squashed, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants