Skip to content

Allow param type definition in route_param#1255

Merged
dblock merged 1 commit into
ruby-grape:masterfrom
namusyaka:allow-param-definition-in-route_param
Jan 25, 2016
Merged

Allow param type definition in route_param#1255
dblock merged 1 commit into
ruby-grape:masterfrom
namusyaka:allow-param-definition-in-route_param

Conversation

@namusyaka

Copy link
Copy Markdown
Contributor

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

@dblock

dblock commented Jan 25, 2016

Copy link
Copy Markdown
Member

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

rngtng commented Feb 1, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

rngtng commented Feb 1, 2016

Copy link
Copy Markdown
Contributor

@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