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

Add possibility for namespace parameters. #204

Merged
merged 3 commits into from
Jul 19, 2012

Conversation

tim-vandecasteele
Copy link
Contributor

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

@dblock
Copy link
Member

dblock commented Jul 18, 2012

This is nice. Take a look at #201 though, we've been talking about getting rid of this params hash and doing a true params DSL. The same thing that you implemented here would probably be solved there in a very different way?

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
```
@tim-vandecasteele
Copy link
Contributor Author

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?

@dblock
Copy link
Member

dblock commented Jul 19, 2012

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.

@tim-vandecasteele
Copy link
Contributor Author

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 desc should not symbolize params, is there a specific reason not to do this?

@dblock
Copy link
Member

dblock commented Jul 19, 2012

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 Hashie::Mash or something like that where strings and symbols are equivalent? Honestly I don't know.

@tim-vandecasteele
Copy link
Contributor Author

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)

dblock added a commit that referenced this pull request Jul 19, 2012
@dblock dblock merged commit b1a8bad into ruby-grape:master Jul 19, 2012
@dblock
Copy link
Member

dblock commented Jul 19, 2012

I've merged this PR, thank you very much.

@tim-vandecasteele
Copy link
Contributor Author

Great, thanks.

BTW, I've published a grape-swagger gem that automatically generates Swagger-compliant documentation from your described grape API.

@dblock
Copy link
Member

dblock commented Jul 19, 2012

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.

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.

None yet

2 participants