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

Add test for declared params with multiple types #2117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Oct 9, 2020

Adds tests from #2112

@grape-bot
Copy link

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#2117](https://github.com/ruby-grape/grape/pull/2117): Add test for declared params with multiple types - [@stanhu](https://github.com/stanhu).

Generated by 🚫 danger

@dblock
Copy link
Member

dblock commented Oct 9, 2020

my question was also whether it matters if we put Hash in requires :bar, type: Array do, so whether we need to have this test try both Hash and Array?

@stanhu
Copy link
Contributor Author

stanhu commented Oct 26, 2020

my question was also whether it matters if we put Hash in requires :bar, type: Array do, so whether we need to have this test try both Hash and Array?

@dblock If I use requires :bar, type: Set, I get:

     Failure/Error: raise Grape::Exceptions::UnsupportedGroupTypeError.new unless Grape::Validations::Types.group?(type)
     
     Grape::Exceptions::UnsupportedGroupTypeError:
       group type must be Array, Hash, JSON or Array[JSON]

It seems that Hash and Array are not valid group types:

# Currently supported group types are Array, Hash, JSON, and Array[JSON]

Do we need a test for this?

@stanhu
Copy link
Contributor Author

stanhu commented Oct 26, 2020

Oh, you were asking about Hash. Let me look at that.

@dblock
Copy link
Member

dblock commented Nov 5, 2020

@stanhu want to finish this?

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.

3 participants