Validation fixes for issue #543 #545
Conversation
|
NOTE: this doesn't pass all specs yet, only the ones in validations_spec - I'll fix the remaining ones if this implementation is what we are looking for. |
| @@ -7,7 +7,7 @@ def validate!(params) | |||
| end | |||
|
|
|||
| def validate_param!(attr_name, params) | |||
| unless params.has_key?(attr_name) | |||
| unless params.respond_to? :'has_key?' and params.has_key?(attr_name) | |||
There was a problem hiding this comment.
This doesn't have to be a :'string'.
2.0.0-p353 :001 > Object.respond_to?(:has_key?)
=> false
2.0.0-p353 :002 > Hash.new.respond_to?(:has_key?)
=> true 
|
This looks good to me. I think that specs need an example of default behavior. |
|
I've added some additional specs for the default behaviour (type defaulting to Array), fixed the remaining specs that were broken by the changes and addressed your review feedback. |
|
Hm, didn't realize that define_method is not valid at the top scope for ruby < 2.0 - will fix that as well. |
| validations[:type] ||= Array if block_given? | ||
| validates(attrs, validations) | ||
|
|
||
| return new_scope(orig_attrs, true, &block) if block_given? |
There was a problem hiding this comment.
Maybe rewrite this as block_given? ? new_scope : push_declared_params(attrs), or even an if/else. A little odd to have a return above the next return.
|
This is very good. Just missing an entry in CHANGELOG, please. If you want to squash the commits in the PR, go for it. Otherwise I can do it when I merge. |
… types * Also adds a :type option to group/requires/optional with block, which can either be Array or Hash. It defaults to Array. * Fixes (2) and (3) as mentioned in ruby-grape#543 * There is a quirk around query parameters: an empty array should pass validation if the array itself is required, but with how query parameters work, it doesn't. It does work fine with body parameters using JSON or similar, which do have a concept of an empty array.
|
That's the squashed all-in-one commit, addressing your last two points (two returns, CHANGELOG). |
|
What's probably worth noting somewhere (other than the query parameter with empty array fun) is that this will break people's stuff if they are currently using |
|
@bwalex Could you contribute an UPGRADING document, lets start listing breaking changes explicitly? I can add the existing things from CHANGELOG. |
|
I did merge this via 5b51c90, but I forgot to ask, we need a README update for the new type option. |
|
Yep, will do both the README update and start an UPGRADING document |
* upstream/master: add posibility to define reusable named params Added ability to restrict declared(params) to the local endpoint with include_parent_namespaces: false. Fix for ruby-grape#464: gracefully handle invalid version headers Rename exception classes to match README. Define errors in context/before blocks. Updated/renamed UPGRADING. README.md and UPGRADE.md update for ruby-grape#543, ruby-grape#545 Address ruby-grape#543 - raise proper validation errors on array/hash types Remove unnecessary test case. Rename children --> subclasses to match the name used in the code. Fix typo in rescue_from unit test (Communication --> Communications). Rescue subclasses in error middleware. Fix for travis-ci/travis-ci#1800. Added Ruby 2.1 support. Lock RSpec version, failing with rspec-expectations 3.x beta.
First and foremost this is a review request, more than a pull request.
Description as per commit message:
which can either be Array or Hash. It defaults to Array.
pass validation if the array itself is required, but with how
query parameters work, it doesn't. It does work fine with body
parameters using JSON or similar, which do have a concept of an
empty array.