Skip to content

ValidationErrors initialize check args[:errors] is present#1259

Open
wuxiaoyi wants to merge 3 commits into
ruby-grape:masterfrom
wuxiaoyi:master
Open

ValidationErrors initialize check args[:errors] is present#1259
wuxiaoyi wants to merge 3 commits into
ruby-grape:masterfrom
wuxiaoyi:master

Conversation

@wuxiaoyi
Copy link
Copy Markdown

ValidationErrors will throw NoMethodError (undefined method `each' for nil:NilClass) when I manually throw a ValidationErrors exception without params.
This argument appears to be necessary, so it should detect the presence or absence of this key. Or give it a default value.
And the fail word should let us know which key is missing.

@dblock
Copy link
Copy Markdown
Member

dblock commented Jan 27, 2016

Thanks!

The first change broke a legit spec. I think it already says "Params are missing", so it's confusing to say "Params are missing: params", no?

The second one looks legit, but in which context do you get it? At least needs a test.

@wuxiaoyi
Copy link
Copy Markdown
Author

@dblock , thank you for your review :-)
I added test and fixes description.

@dblock
Copy link
Copy Markdown
Member

dblock commented Jan 29, 2016

Re-reading all this I have more questions, sorry :)

First, for this implementation. The error should be an ArgumentError. We should also standardize the message matching the rest of Ruby implementations in the core library, so ArgumentError: missing args: errors. If you can please make those changes and squash commits.

Finally, maybe it just makes sense to treat args without errors as an empty array? I am not sure.

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