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

Feature: Allows procs with arity 1 to validate and use custom messages #2333

Conversation

TheDevJoao
Copy link
Contributor

@TheDevJoao TheDevJoao commented Jun 1, 2023

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.

- 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
@TheDevJoao TheDevJoao marked this pull request as ready for review June 1, 2023 21:47
Copy link
Member

@dblock dblock left a 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).
Copy link
Member

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?

Copy link
Contributor Author

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. 😃

CHANGELOG.md Show resolved Hide resolved
@TheDevJoao TheDevJoao changed the title Feature: Allows procs to validate and use custom messages Feature: Allows procs with arity 1 to validate and use custom messages Jun 2, 2023
@TheDevJoao TheDevJoao requested a review from dblock June 2, 2023 22:12
Copy link
Member

@dblock dblock left a 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'
Copy link
Member

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?

Copy link
Contributor Author

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.

@dblock
Copy link
Member

dblock commented Jun 4, 2023

@TheDevJoao add a test for when arity is > 1 and we can merge?

@TheDevJoao
Copy link
Contributor Author

TheDevJoao commented Jun 5, 2023

@TheDevJoao add a test for when arity is > 1 and we can merge?

@dblock Done! 🤝

@TheDevJoao TheDevJoao requested a review from dblock June 5, 2023 14:42
@dblock dblock merged commit d1dfdcc into ruby-grape:master Jun 5, 2023
23 checks passed
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.

set custom message when using proc to validate value
2 participants