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

[Bug] Fix validations nested under a given #1691

Conversation

rnubel
Copy link
Contributor

@rnubel rnubel commented Oct 6, 2017

Addresses #1681.

Multi-param and default validators, all of which implemented their own validate! method, were not respecting the given dependency as expected. This change copies the check into all those validators.

The bug was likely introduced by #1625, which required that the dependency check be moved out of the scope itself and into the validators. Rolling back that change would require fundamentally re-architecting how array parameters are validated (possibly with a new kind of scope?).

@rnubel rnubel force-pushed the fix/multiple-param-and-default-validators-with-given branch from e48a8e4 to b631c02 Compare October 6, 2017 15:52
@dblock
Copy link
Member

dblock commented Oct 6, 2017

This needs a rubocop -a ; rubocop --auto-gen-config and ^ as well.

Multi-param and default validators, all of which implemented their
own `validate!` method, were not respecting the `given` dependency
as expected. This change copies the check into all those validators.

The bug was likely introduced by ruby-grape#1625, which required that the
dependency check be moved out of the scope itself and into the
validators. Rolling back that change would require fundamentally
re-architecting how array parameters are validated (possibly with
a new kind of scope).
@rnubel rnubel force-pushed the fix/multiple-param-and-default-validators-with-given branch from b631c02 to fe56f15 Compare October 10, 2017 20:19
@dblock
Copy link
Member

dblock commented Nov 1, 2018

Bump @rnubel ?

@rnubel
Copy link
Contributor Author

rnubel commented Nov 2, 2018

@dblock #1755 actually resolved this one; I just verified that locally by running the specs I added for this PR against master's code, and they pass.

@rnubel rnubel closed this Nov 2, 2018
@rnubel rnubel deleted the fix/multiple-param-and-default-validators-with-given branch November 2, 2018 03:59
@dblock
Copy link
Member

dblock commented Nov 2, 2018

Great. Double check please that there's a spec for this exact scenario and PR that if not. Appreciate it.

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.

2 participants