-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add possibility for namespace parameters. #204
Add possibility for namespace parameters. #204
Conversation
This is nice. Take a look at #201 though, we've been talking about getting rid of this |
Specific method options (parameters and description) are now merged with the namespace parameters. So you can do something like below, and the serial parameter will be described for all methods in the namespace: ``` desc 'description', :params => { "serial" => { :desc => "Serial of requested object." } } segment '/:serial' do desc "Get details on specific object" get do present object, with: API::Entities::Object end desc "Reserve an object" post '/reserve' do present object.reserve, with: API::Entities::ObjectReserved end end ```
I saw #201 is merged. It doesn't change anything for this pull request. I rebased my branch without any issues. Functionality is still ok, and tests are still passing. Are there other upcoming changes you expect to conflict with this? |
Yes, the PR is fine technically. But I don't think we should merge it. Rather we should deprecate this params syntax and re-implement what you're suggesting with the newer syntax from #201. desc "Does something"
params do
required :name, :desc=> "Tour name"
required :numbers, :desc => "Numbers", :coerce => Array[Integer]
optional :age, :desc => "Your Age"
end If that's the case, we don't want namespaces to get additional metadata in what you're proposing here, but in the syntax above. Would like your opinion, please. |
I can update the code so it adheres to this new syntax, actually there's not really much to change except the tests. There's one problem I've encountered though: In endpoint.rb it says: named_params = regex.named_captures.map { |nc| nc[0] } - [ 'version', 'format' ]
named_params.each { |named_param| path_params[named_param] = "" } but in the validation it says @last_description[:params][name.to_sym] ||= {}
@last_description[:params][name.to_sym].merge!(opts) Meaning if you describe a parameter and it's in the description/validation, it appears double in the params hash ("name" and :name) I see in the spec there's an explicit check that |
Cool, thanks. I would first find a clean way to deprecate the old parameter style - possibly by just deleting the code. It will be a lot less work for everybody. To answer your question, I am going to guess that it's because you can have parameters that cannot be symbolized. Maybe we want that has to become a |
…e in the params hash (once with symbol key, once with string key)
I adapted the code in the spec to the new DSL, implementation remains the same, as it's still the same hash that gets filled. I also changed the code in the validation to avoid the problem I described above. Included it in this pull request, but actually it doesn't have anything to do with the merging of namespace descriptions (if you want I can submit this fix separately) |
I've merged this PR, thank you very much. |
Great, thanks. BTW, I've published a grape-swagger gem that automatically generates Swagger-compliant documentation from your described grape API. |
Tim, you should totally write a blog post about this and I'll do my best to share. Tell the grape mailing list at least. |
Specific method options (parameters and description) are now merged with the namespace parameters. So you can do something like below, and the serial parameter will be described for all methods in the namespace: