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

Allow param type definition in route_param #1255

Merged

Conversation

namusyaka
Copy link
Contributor

ref #1009
I'd like to use type definition in route_param, so I implemented it by using requires DSL.

@dblock
Copy link
Member

dblock commented Jan 25, 2016

Very nice thanks.

dblock added a commit that referenced this pull request Jan 25, 2016
…te_param

Allow param type definition in route_param
@dblock dblock merged commit 5712b5b into ruby-grape:master Jan 25, 2016
@rngtng
Copy link
Contributor

rngtng commented Feb 1, 2016

Great feature, unfortunately this breaks with grape_logging middleware, due to nil value for Response object which is required here. Nevertheless, the fact that response is nil, should be solved in grape. Any idea why this happens?

In a very simplified version, my migration looks like:

params do
  requires :id, type: Integer
end
get '/:id' do
  params.to_json
end
route_param :id, type: Integer do
  get do
    params.to_json
  end
end

@namusyaka
Copy link
Contributor Author

@rngtng Could you provide a minimal failing project with Gemfile.lock?

@namusyaka namusyaka deleted the allow-param-definition-in-route_param branch February 1, 2016 12:03
@rngtng
Copy link
Contributor

rngtng commented Feb 1, 2016

@namusyaka thx for you quick response. while creating an example project i figured it actually hasn't anything todo with the refactoring to route_param, but with upgrading to grape HEAD. This pulled in
#1240 which call after on logging https://github.com/aserafin/grape_logging/blob/master/lib/grape_logging/middleware/request_logger.rb#L18. let's continue discussion on #1240

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