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

extra_params use Hash#deep_merge! instead of merge #182

Merged
merged 3 commits into from
Dec 9, 2014
Merged

extra_params use Hash#deep_merge! instead of merge #182

merged 3 commits into from
Dec 9, 2014

Conversation

jsmestad
Copy link
Contributor

@jsmestad jsmestad commented Dec 5, 2014

This allows support for this use case:

post '/post' do
  parameter :title, 'Title of post', scope: :post
  parameter :author_id, 'Integer of author', scope: :post

  let(:title) { 'My First Post' }
  let(:author_id) { '8' }

  example_request 'returns successful' do
    expect(response_body).to have_key(:id)
  end

  example_request 'fails with malformed id', post: {author_id: 'asdf'} do
    expect(response_body).to have_key(:errors)
  end
end

Before this change the params changes from post: {title: 'My First Post', author_id: 'asdf'} to post: {author_id: 'asdf'}. Hash#deep_merge! resolves this.

This allows support for this use case:

```ruby
post '/post' do
  parameter :title, 'Title of post', scope: :post
  parameter :author_id, 'Integer of author', scope: :post

  let(:title) { 'My First Post' }
  let(:author_id) { '8' }

  example_request 'returns successful' do
    expect(response_body).to have_key(:id)
  end

  example_request 'fails with malformed id', post: {author_id: 'asdf' } do
    expect(response_body).to have_key(:errors)
  end
end
```

Before this change the params changes from `post: {title: 'My First Post', author_id: 'asdf'}` to `post: {author_id: 'asdf'}`. `Hash#deep_merge!` resolves this.
@oestrich
Copy link
Contributor

oestrich commented Dec 5, 2014

Awesome, didn't know about deep_merge. Can you add a test for this? I'll go ahead and merge it after that.

@jsmestad
Copy link
Contributor Author

jsmestad commented Dec 5, 2014

@oestrich I think I still have one issue I need to resolve. Let me verify and I will ping you when its 👍

@jsmestad
Copy link
Contributor Author

jsmestad commented Dec 5, 2014

@oestrich alright this one should be good to go. The issue I was hitting is reported in #183 if you can look at that as well, but its unrelated to this patch.

@jsmestad
Copy link
Contributor Author

jsmestad commented Dec 8, 2014

@oestrich spec added to verify working order. I did discover that extra_params method did not handle the case where v is a Hash and not a direct value. That is fixed as well.

👍

oestrich added a commit that referenced this pull request Dec 9, 2014
extra_params use Hash#deep_merge! instead of merge
@oestrich oestrich merged commit 2f1fe19 into zipmark:master Dec 9, 2014
@oestrich
Copy link
Contributor

oestrich commented Dec 9, 2014

Looks good. Thanks!

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.

2 participants