Skip to content

Fix logic for include_parent_namespaces option #1415

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

Closed

Conversation

304
Copy link
Contributor

@304 304 commented Jun 6, 2016

Based on PR #1408.

  • Add specs to test different combinations of namespaces
  • Add documentation for the include_parent_namespaces option

all_declared_params = route_setting(:declared_params)
current_namespace_declared_params = route_setting(:saved_declared_params).last

declared_params ||= options[:include_parent_namespaces] ? all_declared_params : current_namespace_declared_params
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it won't harm to provide a short explanation what is going on here.

route_setting(:declared_params) keep every parameter in a flat array, for example:

route_setting(:declared_params) # [:parent_name, :child_name, :child_age, :grandchild_name]

However, route_setting(:saved_declared_params) stores parameters in groups by namespace.

route_setting(:saved_declared_params) # [[:parent_name], [:child_name, :child_age], [:grandchild_name]]

The last element here contains only parameters that were defined in current namespace.

@dblock
Copy link
Member

dblock commented Jun 7, 2016

I like this. This just needs a CHANGELOG entry, please.

@304
Copy link
Contributor Author

304 commented Jun 8, 2016

@dblock, CHANGELOG updated => 8a7a24c

@@ -15,6 +15,7 @@
* [#1365](https://github.com/ruby-grape/grape/pull/1365): Fix finding exception handler in error middleware - [@ktimothy](https://github.com/ktimothy).
* [#1380](https://github.com/ruby-grape/grape/pull/1380): Fix `allow_blank: false` for `Time` attributes with valid values causes `NoMethodError` - [@ipkes](https://github.com/ipkes).
* [#1384](https://github.com/ruby-grape/grape/pull/1384): Fix parameter validation with an empty optional nested `Array` - [@ipkes](https://github.com/ipkes).
* [#1415](https://github.com/ruby-grape/grape/pull/1415): Fix logic for `include_parent_namespaces` option - [@304](https://github.com/304).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't say where, it's in declared, maybe say "Fix declared(params, include_parent_namespaces: true)"?

faucct and others added 3 commits June 8, 2016 13:11
Fixed the tests to point that out.
Based on PR ruby-grape#1408.

* Add specs to test different combinations of namespaces

* Add documentation for the `include_parent_namespaces` option
@304 304 force-pushed the fix_include_parent_namespaces_behaviour branch from 8a7a24c to 497c30d Compare June 8, 2016 11:13
@dblock
Copy link
Member

dblock commented Jun 8, 2016

Merged via 7d1b3e3.

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