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

Feature: add group attributes for parameter definitions #1507

Merged
merged 2 commits into from
Oct 12, 2016

Conversation

304
Copy link
Contributor

@304 304 commented Oct 7, 2016

It is a proposal with a working prototype for a group attributes feature. If it makes sense then I will continue working on this PR. Your feedback is welcome.

I was looking into this issue (#1482) recently. I think the presented use case is common for complex APIs schemas. Currently, if I want to apply the same validator (or the same type) for a several endpoint attributes, then I have to define it for every parameter.

What if we introduce a way of defining common parameter settings? Something like Rails conditional validations.

For example, this parameters definition:

params do
  requires :first_name, type: String, allow_blank: false
  requires :last_name, type: String, allow_blank: false
end

can be replaced with

params(with: { type: String, allow_blank: false }) do
  requires :first_name
  requires :last_name
end

It is also possible to extract group attributes into a class method and use it instead of default params in your code. It should provide the solution for the problem from #1482.

class MyAPI < Grape::API do
  def self.not_blank_params(&block)
    params(with: { allow_blank: false }, &block)
  end

  not_blank_params do
    requires :first_name # This param won't accept blank lines 
    requires :last_name, allow_blank: true # This param *will* accept blank lines. 
  end
end

/cc @dblock

@rnubel
Copy link
Contributor

rnubel commented Oct 7, 2016

I think the use case where all parameters for an endpoint share the same options is pretty rare; even if your initial endpoint has only homogenous params, eventually you'll want to add an extra param of a different type and be forced to undo all the shared options. However, like you point out, having a group of parameters with shared options is very common. What about a solution which introduces a new method within the params DSL? Something like this:

params do
  with(type: String, allow_blank: false) do
    requires :first_name
    requires :last_name
  end

  requires :age, type: Integer, allow_blank: true
end

@dblock
Copy link
Member

dblock commented Oct 7, 2016

Thanks @304, this is great. I think @rnubel's suggestion is spot on and is even better, since this gives us a proper DSL to deal with within params and something much more extensible for the future. I'd definitely take that PR.

@dgasper
Copy link
Contributor

dgasper commented Oct 7, 2016

@rnubel I guess this should already be possible like this:

params do
  requires :first_name, :last_name, type: String, allow_blank: false
  requires :age, type: Integer, allow_blank: true
end

@jlfaber
Copy link
Contributor

jlfaber commented Oct 7, 2016

You can use an array of attribute names rather than a single one to define a bunch of identical attributes, right?

params do
  requires %i(first_name last_name), type: String, allow_blank: false
end

One could also use a params helper to dynamically specify the name(s) of attrs that need a specific validation.

helpers do
  params :name do |options|
    optional options[:attr], type: String, allow_blank: false
  end
end

params do
  use :name, attr: :first_name
  use :name, attr: :last_name
end

@304
Copy link
Contributor Author

304 commented Oct 12, 2016

Thank you for the feedback! I like @rnubel's suggestion too. I think it is better to have it this way than as a part of params method. I will try to implement it and I will update this PR when I have something to show.

@304
Copy link
Contributor Author

304 commented Oct 12, 2016

It is ready. I've added a new method with to Grape DSL. It allows to extract and group common options for a list of parameters. The documentation was also updated. Can you please take a look?

/cc @dblock

@dblock
Copy link
Member

dblock commented Oct 12, 2016

I'm going to merge this. Seems like a sensible add-on and doesn't hurt at the very least. If anyone hates it or thinks it can be improved further, speak up so we can iterate before the next release.

@dblock dblock merged commit 3a34ca2 into ruby-grape:master Oct 12, 2016
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.

5 participants