Add the ability for parameter values to accept a Proc

[danhawkins]

I added this to fix a small problem of my own, thought it might be useful to the main project. I've added tests and README section

[dblock]

Please update the CHANGELOG, too.

[dblock]

Values are evaluated above.

[dm1try]

@dblock , @danhawkins how can I help to move on this PR?

[dblock]

• in lib/grape/validations.rb there's still the issue of double evaluating the value, see https://github.com/intridea/grape/pull/558/files#r9147321
• Travis-CI is failing, likely because of something in this change, and needs to be fixed

[dblock]

It would be nice if the example in the README was related to the general topic of Twitter feed/status and were actually "useful". Right now the example you added just returns a fixed array, so it's unclear why anyone should be using a Proc in that case.

[danhawkins]

The README has an example for specifying values, I only copied that to illustrate a spec, I am not sure of something that could be validated like this using twitter examples

[dblock]

Fyi, fixed the weird build error in master (was introduced by a Rubocop upgrade), you should rebase.

[dblock]

OK on the values example, but you can see how required :status, type: Symbol, values: -> { [:not_started, :processing, :done] } is actually just adding the lambda for the sake of using a lambda. Find an example where the lambda is actually useful.

[dm1try]

in lib/grape/validations.rb there's still the issue of double evaluating the value, see https://github.com/intridea/grape/pull/558/files#r9147321

@dblock, we can don't touch values_validator.rb and prepare values in validates method:

 values = validations[:values]
 values = (values.is_a?(Proc) ? values.call : values)
 validations[:values] = doc_attrs[:values] = values if values

Does this smell good?) (I think it's better then the double evaluating and can be encapsulated in more generic way)

[dblock]

Do you think it would be a bad idea to execute the proc at runtime for every incoming request as well? Like this code.

[dblock]

Merged as 764f4c0. I made a couple small changes, please check.

[dblock]

The https://github.com/wpschallenger/grape/commit/1e05a437ad251fae91efd737dd172e2ef16b4dca change didn't make it in my merge. If you think it's a good idea, start a new PR, we'll need a test that behaves differently depending on when the values.call is evaluated. I do think it probably makes more sense.

[dblock]

And thanks for being patient with me.

[danhawkins]

Sure, I will add that later on today, when a dynamic call is added later.

Regards

Danny Hawkins

[email protected]
mobile: +44 (0)7447 158411
skype: dannyhawkins81
jabber: [email protected]
linkedin: http://www.linkedin.com/in/dannyhawkins

On 28 January 2014 at 16:58:35, Daniel Doubrovkine (dB.) ([email protected]) wrote:

The wpschallenger@1e05a43 change didn't make it in my merge. If you think it's a good idea, start a new PR, we'll need a test that behaves differently depending on when the values.call is evaluated.

—
Reply to this email directly or view it on GitHub.

[dm1try]

Ok, if default and values will be evaluated at runtime, those conditions should be evaluated at runtime too:

if default && values && !values.include?(default)
  raise Grape::Exceptions::IncompatibleOptionValues.new(:default, default, :values, values)
end

# type should be compatible with values array, if both exist
if coerce_type && values && values.any? { |v| !v.instance_of?(coerce_type) }
  raise Grape::Exceptions::IncompatibleOptionValues.new(:type, coerce_type, :values, values)
end

[dblock]

Looking forward to the next pr, @danhawkins, thx.

[dm1try]

But for me, it's bad idea to generate default and values at runtime for every incoming request. These fields are contract between me and client, like default and values in any public API, for example https://trello.com/docs/api/action/index.html (we also generate docs using these fields..). If these fields changed well then API was changed.
I don't mind if values are stored in a db like const enum, it's ok.

IMHO, all app context specific restrictions(model validation) should be executed at processing endpoint. Maybe I misunderstood something.

[danhawkins]

I get your point, but in my case using grape I have dynamic values which I want to validate, and using grape-swagger I want to see those update also. The benefit of this approach is that the developer can choose to use dynamic values or not, with negligible performance implications.
On 28 January 2014 at 19:33:59, dm1try ([email protected]) wrote:

But for me, it's bad idea to generate default and values at runtime for every incoming request. These fields are contract between me and client, like default and values in any public API, for example https://trello.com/docs/api/action/index.html (we also generate docs using these fields..). If these fields changed well then API was changed.
I don't mind if values are stored in a db like const enum, it's ok.

IMHO, all app context specific restrictions(model validation) should be executed at processing endpoint. Maybe I misunderstood something.

—
Reply to this email directly or view it on GitHub.

[dblock]

@danhawkins Isn't the lambda unnecessary in the context where you want to evaluate the values once, and then just check against that? Lets take the example of this PR:

required :hashtag, type: String, values: -> { Hashtag.all.map(&:tag) }

Isn't this the same as

required :hashtag, type: String, values: Hashtag.all.map(&:tag)

In fact, the specs in values_spec right now would succeed the same whether you use lambda or not. So what's merged looks just like syntax sugar.

[danhawkins]

Hey,

Yes, thats right, right now it doesn’t do what is needed, but I have a branch I am working on to actually realise the solution. I imagine I will submit the pull request tomorrow, but it has been a little more tricky than I first planned!

On 28 January 2014 at 21:12:26, Daniel Doubrovkine (dB.) ([email protected]) wrote:

@danhawkins Isn't the lambda unnecessary in the context where you want to evaluate the values once, and then just check against that? Lets take the example of this PR:

required :hashtag, type: String, values: -> { Hashtag.all.map(&:tag) }
Isn't this the same as

required :hashtag, type: String, values: Hashtag.all.map(&:tag)
In fact, the specs in values_spec right now would succeed the same whether you use lambda or not. So what's merged looks just like syntax sugar.

—
Reply to this email directly or view it on GitHub.