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

Fixed the range :values options, now it does not fail with float range #234

Merged
merged 1 commit into from
Mar 18, 2015

Conversation

azhi
Copy link
Contributor

@azhi azhi commented Mar 17, 2015

now range values is exposed as array in enum field only if it is Integer or
String range
float range or procs returning non-array values is exposed as string
fixes #233

@azhi azhi force-pushed the fix-param-values-float-range-error branch from 00627be to 20b87e9 Compare March 17, 2015 15:29
@azhi
Copy link
Contributor Author

azhi commented Mar 17, 2015

Grape below 0.11.0 has same bug: it tried to iterate through range without checking if it can be done.
It was fixed with some values validating regression fix (see ruby-grape/grape@0b98900).
Build for all grape versions below 0.11.0 will fail.
Not sure what should we do about this.

@dblock
Copy link
Member

dblock commented Mar 18, 2015

The tests for these older versions of Grape should be skipped. I think you can just condition the tests on the version of Grape and I would be ok to merge.

@@ -9,6 +9,7 @@
#### Fixes

* [#232](https://github.com/tim-vandecasteele/grape-swagger/pull/232): Fixed missing raw array params - [@u2](https://github.com/u2).
* [#234](https://github.com/tim-vandecasteele/grape-swagger/pull/234): Fixed the range :values options, now it does not fail with float range - [@azhi](https://github.com/azhi).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe simpler: "Fixed range :values with float" or something like that?

@dblock
Copy link
Member

dblock commented Mar 18, 2015

As you pointed out in #233 you cannot be converting ranges into arrays - it would be eating a lot of memory and take a very long time. We need a better solution.

 now range values is exposed as array in enum field only if it is Integer or
 String range
 float range or procs returning non-array values is exposed as string
 fixes ruby-grape#233
@azhi azhi force-pushed the fix-param-values-float-range-error branch from 20b87e9 to 7b5573d Compare March 18, 2015 17:07
@azhi
Copy link
Contributor Author

azhi commented Mar 18, 2015

Updated.

@dblock i think the proper solution would be not to convert ranges to enums at all. However, this feature was in the release and some people might be using it.

Also we could try some dirty hack like convert to an array only if Range#size is less then some amount (say, 50). But that seems very odd to me.

@dblock
Copy link
Member

dblock commented Mar 18, 2015

I am going to merge this. Could you open a new issue regarding the Range problem? Thanks.

dblock added a commit that referenced this pull request Mar 18, 2015
 Fixed the range :values options, now it does not fail with float range
@dblock dblock merged commit 828b285 into ruby-grape:master Mar 18, 2015
@azhi azhi deleted the fix-param-values-float-range-error branch March 18, 2015 18:22
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.

Params values option float range and enums
2 participants