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

Allows values to be a kind of the coerced type in range #885

Closed
wants to merge 1 commit into from
Closed

Allows values to be a kind of the coerced type in range #885

wants to merge 1 commit into from

Conversation

u2
Copy link
Contributor

@u2 u2 commented Jan 8, 2015

My code like this ``requires :month, type: Integer, values: 1..12`, the Range values coercion is workable.Before it workable, now it failed.

https://github.com/u2/grape/commit/0df3bda5d288d03d1647170ab2cbcb645001a324#diff-45b62b4ed0aed7ceff346c347f76570e

After !(@values.is_a?(Proc) ? @values.call : @values).include?(params[attr_name]) chang to (param_array - values).empty?, the Range values coercion raise a error no implicit conversion of Range into Array

@u2 u2 changed the title allows values to be a kind of the coerced type in range Allows values to be a kind of the coerced type in range Jan 8, 2015
@dblock
Copy link
Member

dblock commented Jan 8, 2015

I think we should just support Range explicitly with code that looks at values and says "are you a range, if so do this ...". As you pointed out having a big range would make a side effect of converting it to an array every time, which can't be good ;)

This will need a CHANGELOG entry and a note in the README that Range is explicitly supported as a type.

@dblock
Copy link
Member

dblock commented Jan 8, 2015

This is #881.

@u2
Copy link
Contributor Author

u2 commented Jan 9, 2015

I think it's easy to check a value in a Range or not.But it's not easy to check the array params in a Range or not, unless converting it to an array every time.

@dblock
Copy link
Member

dblock commented Jan 9, 2015

Can we make this lazier though? At least half of the use-case is not an Array/Range scenario.

@ajvondrak
Copy link
Contributor

BTW, I think this can be closed since #899 was merged.

@u2
Copy link
Contributor Author

u2 commented Jan 22, 2015

Thanks.

@dblock
Copy link
Member

dblock commented Jan 22, 2015

Thanks, closing.

@dblock dblock closed this Jan 22, 2015
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