refactor declared and include nil values for missing nested params #1575
Conversation
thogg4
changed the title
thogg4
changed the title
thogg4
changed the title
|
@dblock Tests are passing now. Can I get some feedback? |
spec/grape/endpoint_spec.rb
Outdated
| end | ||
| end | ||
| end | ||
|
|
||
| it 'should show nil for nested params if include_missing and include_missing_nested are true' do |
There was a problem hiding this comment.
There's no more include_missing_nested.
| @@ -26,42 +26,56 @@ def self.post_filter_methods(type) | |||
| # Methods which should not be available in filters until the before filter | |||
| # has completed | |||
| module PostBeforeFilter | |||
| def declared(params, options = {}, declared_params = nil) | |||
| def declared(passed_params, options = {}, declared_params = nil) | |||
There was a problem hiding this comment.
Maybe unrename passed_params to just params, will be easier to grok.
There was a problem hiding this comment.
I actually added this in because I thought it made it a little easier to grok.
When this is called and the declared child params params are passed as an argument, its a bit easier to remember that passed_params are always the params passed into the method and declared_params are the params that were declared in the params block
lib/grape/dsl/inside_route.rb
Outdated
|
|
||
| key.each_pair do |parent, children| | ||
| output_key = options[:stringify] ? parent.to_s : parent.to_sym | ||
| def declared_array(passed_params, options, declared_params) |
There was a problem hiding this comment.
These should probably be private.
lib/grape/dsl/inside_route.rb
Outdated
| next unless options[:include_missing] || params.key?(parent) | ||
| def declared_hash(passed_params, options, declared_params) | ||
| declared_params.each_with_object(Hashie::Mash.new) do |declared_param, memo| | ||
| # if it is not a Hash then it does not have children. |
There was a problem hiding this comment.
Nitpick, this comment should not have a period if it's not a full sentence starting with a Capital.
spec/grape/endpoint_spec.rb
Outdated
| it 'should show nil for nested params if include_missing and include_missing_nested are true' do | ||
| inner_params = nil | ||
| subject.get '/declared' do | ||
| inner_params = declared(params, include_missing: true) |
There was a problem hiding this comment.
I realize we do this elsewhere, but it's bad practice - maybe fix this and other instances?
This works, but is hacky and abuses the fact that the inner function still has access to outer variables. Below you check that the GET call succeeded with a 200, but then reach in for the value of inner_params. Be consistent, return declared(...) and examine the JSON response instead of relying on this.
|
I have fairly cosmetic change requests, otherwise this looks OK. Mostly trusting the fact that no existing specs broke :) |
|
@dblock Thanks for your comments, I have done the majority of those things. Please see my comment about naming |
|
Great work @thogg4, merged. |
|
This change breaks one of our existing applications. We are using |
addresses #1574