-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Feature: Allows procs with arity 1 to validate and use custom messages #2333
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
Feature: Allows procs with arity 1 to validate and use custom messages #2333
Conversation
- now allows a proc to validate and use custom messages - adds @proc_message instance variable - creates the skip_validation? method to pass rubocops cyclomatic error
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.
I don't see :proc_message
anywhere in specs, how is this wired up?! I must be missing something obvious 😅
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
#### Features | |||
|
|||
* [#2333] (https://github.com/ruby-grape/grape/pull/2333): The `validate_param!` now allows procs to validate and use custom messages [@thedevjoao](https://github.com/TheDevJoao). |
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.
I think "Use custom messages in parameter validation." may be clearer?
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.
I don't see
:proc_message
anywhere in specs, how is this wired up?! I must be missing something obvious sweat_smile
Hey @dblock , thanks for the review! Your comment helped out a ton, turns out what I was doing wasn't necessary. I recently refactored the code to cover what was suggested here and it should make more sense as an added feature. 😃
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.
This makes a lot more sense now!
when 1 | ||
param_array.all? { |param| proc.call(param) } | ||
else | ||
raise ArgumentError, 'proc arity must be 0 or 1' |
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.
Do we get some kind of useful call stack with a line number such as that the developer knows where the error comes from?
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.
Yeah, considering the ruby and rails versions that grape supports, the stack trace will show every sequence leading to this raise statement.
@TheDevJoao add a test for when arity is > 1 and we can merge? |
@dblock Done! 🤝 |
This PR resolves #2280
As this is my first contribution to the repo, I am open to any form of feedback!
PS: I also created the tests with the example shown in the issue in mind.