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

Find coerce_type for Array when not specified #826

Merged
merged 1 commit into from
Dec 1, 2014
Merged

Find coerce_type for Array when not specified #826

merged 1 commit into from
Dec 1, 2014

Conversation

manovotn
Copy link
Contributor

This PR is an attempt to address issue #795 .
I am not sure I understood it correctly, but it seemed to me that @maxd wanted to create an Array type without specifying what's inside the Array.
I also added one test to make sure this works.

@dblock
Copy link
Member

dblock commented Nov 27, 2014

I think this is good. It needs a CHANGELOG entry and definitely more tests: calling the API with an invalid value to start.

@manovotn
Copy link
Contributor Author

I updated changelog and added some tests.
However one of the tests is failing and I could use some advice on how to resolve this. I am a Ruby beginner so it is likely something trivial...
This is the failing test. It goes wrong when it gets into values.rb and tries to pass this condition. To be more exact, param_array contains String instead of Integer/Fixnum and I got honestly no clue why.

This gets resolved (and all tests pass) if I remake the condition to this:

unless (param_array.map { |n| n.to_s} - values.map { |n| n.to_s}).empty? 

However it leads to always comparing based on string representation. I see that coercion check is done before code reaches this validation but I am not sure it would be valid change.

@dblock could you please comment on it and point me in the right direction?

@dblock
Copy link
Member

dblock commented Nov 29, 2014

Before I answer this, is this even possible:

post '/foo', values: [1, 2, 3]

Imagine this is outside a client, you can POST JSON, some kind of parseable URI (eg. values[]=1&values[]=2&values[]=3), but not that, no?

@manovotn
Copy link
Contributor Author

So you think that one test is pointless?
Are there other tests I should add then?

@dblock
Copy link
Member

dblock commented Nov 30, 2014

Right, I think it's not a realistic scenario, works only under RSpec.

@manovotn
Copy link
Contributor Author

manovotn commented Dec 1, 2014

Ok, I removed the test case, fixed Rubocop offenses and rebased against upstream/master. Is it alright now?

@dblock
Copy link
Member

dblock commented Dec 1, 2014

Yes, this is good. Would you please squash your commits, too? I'll merge.

@manovotn
Copy link
Contributor Author

manovotn commented Dec 1, 2014

Done.

dblock added a commit that referenced this pull request Dec 1, 2014
Find coerce_type for Array when not specified
@dblock dblock merged commit 0b15f54 into ruby-grape:master Dec 1, 2014
@dblock
Copy link
Member

dblock commented Dec 1, 2014

Thanks, merged.

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