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

Ability to set nested scopes #221

Merged
merged 2 commits into from
Aug 11, 2015
Merged

Ability to set nested scopes #221

merged 2 commits into from
Aug 11, 2015

Conversation

kevintraver
Copy link
Contributor

Solves #176 and #198

@cover
Copy link
Contributor

cover commented Aug 9, 2015

I was thinking on this issue as well and I've came up with the following:

def set_param(hash, param)
  key = param[:name]
  return hash if in_path?(key)

  keys = [param[:scope], key].flatten.compact
  method_name = keys.join('_')

  unless respond_to?(method_name)
    method_name = key
    return hash unless respond_to?(method_name)
  end

  hash.deep_merge(build_param_hash(keys, method_name))
end

def build_param_hash(keys, method_name)
  value = keys[1] ? build_param_hash(keys[1..-1], method_name) : send(method_name)
  { keys[0].to_s => value }
end

What do you think? This way we can keep the current way to set values without breaking any existing app:

parameter :quantity, 'Number', scope: :order
let(:quantity) { 42 }

Or the scoped version when there are different parameters with the same name:

parameter :quantity, 'Number', scope: [:data, :attributes] # OR [:data, [:attributes]] OR {data: :attributes}
let(:data_attributes_quantity) { 42 }

I've used the flatten method in order to have deep arrays and the single hash level working ( { data: { attributes: :something } } doesn't work). Otherwise it could be removed and specify in the docs that it only works with arrays and not hashes

All existing tests (rspec / cucumber) are green

@kevintraver
Copy link
Contributor Author

Looks like there is some issues now with the documentation output for the parameters with nested scope.

Specifically parameter description and required label.

@cover
Copy link
Contributor

cover commented Aug 9, 2015

Are you referring to the travis break?

Checking on the travis history it seems that the same failed tests are present also in the last master commit, which only edits the README, and the previous commit fails with a different error, this doesn't make a lot of sense 😕

@kevintraver
Copy link
Contributor Author

No, this is a different issue. The failing tests on master are unrelated. @oestrich

@cover
Copy link
Contributor

cover commented Aug 9, 2015

What issues are you having?

I've tried to generate the doc with some scopes on a project I'm working on and I see from the generated json files that the only difference is that the scope is an array (as expected), and that apitome doesn't show it correctly (eg: ["data", "attributes"][body] , instead of data[attributes][body]), but that'd need to be patched on apitome (I guess on raddocs as well, I haven't tried it).

Both the description and the required label are shown correctly though..

@kevintraver
Copy link
Contributor Author

Yeah, the html could be cleaned up a little, and apitome and raddocs would need to be updated as well, but this is a good start.

@oestrich
Copy link
Contributor

This looks good, I think it should cover everything. I'll update the documentation after merging this along with updating Raddocs. I don't think it breaks anything either so I can release 4.5.0 soon.

@oestrich
Copy link
Contributor

It passes locally for me, not sure what's up with travis so I'm good on that end.

oestrich added a commit that referenced this pull request Aug 11, 2015
@oestrich oestrich merged commit 5f55530 into zipmark:master Aug 11, 2015
This was referenced Aug 11, 2015
@kevintraver kevintraver deleted the nested-scope branch August 11, 2015 19:00
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