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.

Sign up for GitHub

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

Jump to bottom

Fix shared params with exactly_one_of #1755

Merged
merged 1 commit into from
Mar 28, 2018
Merged

Fix shared params with exactly_one_of #1755

merged 1 commit into from
Mar 28, 2018

Conversation

milgner
Copy link
Contributor

@milgner milgner commented Mar 28, 2018

Tentative fix for #1288 - not sure whether it is the right approach but the specs are green 😉

I can't shake the feeling that there's some unnecessary complexity in combination with the changes from #1625 so I'm open to suggestions.

@milgner
Copy link
Contributor Author

milgner commented Mar 28, 2018

Alrighty, waiting for Travis to do its thing and if everything is green there as well, I'll update the changelog...

@rnubel
Copy link
Contributor

rnubel commented Mar 28, 2018

It's been a while since I looked into this, but I like the simplicity of your fix. My own attempt at resolving the bug is https://github.com/ruby-grape/grape/pull/1691/files, but I don't like how it involved updating all of the validators individually (which is why I stalled on it). However, the specs that I added in that PR might be useful as additional test cases for this PR.

dblock
dblock reviewed Mar 28, 2018
View reviewed changes
spec/integration/multi_xml/xml_spec.rb Outdated
@@ -1,7 +1,13 @@
require 'spec_helper'

describe Grape::Xml do
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, integration specs run in isolation. Is this failing on HEAD?

@dblock
Copy link
Member

dblock commented Mar 28, 2018

Fix looks good to me. The XML adapter spec should be undone (run rake for tests) otherwise it's all good.

@milgner
Copy link
Contributor Author

milgner commented Mar 28, 2018

Sorry, I was using rspec to run the tests and it was failing, so I fixed it. But using rake, it works as expected so I dropped the commit from the branch again.

@dblock dblock merged commit 3cd7f86 into ruby-grape:master Mar 28, 2018
@rnubel rnubel mentioned this pull request Nov 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@milgner @rnubel @dblock