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

added note for mutually_exclusive params #1720

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Conversation

gencer
Copy link
Contributor

@gencer gencer commented Dec 18, 2017

No description provided.

@dblock
Copy link
Member

dblock commented Dec 19, 2017

This is related to #1717. I think this note makes it even more confusing, what should happen is that if you specify a default for more than one parameter (or any parameter) in a mutually exclusive relationship you should get an error on mounting the API.

@gencer
Copy link
Contributor Author

gencer commented Dec 20, 2017

But, I am sure people will fall into this if they simply forget :default in their optional params when they are using exactly_one_of. To prevent this, and let them know that those params should not exists even as a nil, we should add a note/warning.

Still, It is your decision of course :)

@dblock
Copy link
Member

dblock commented Dec 20, 2017

I just think the language of the note is a bit confusing and unnecessarily alarming :) Maybe make it consistent with other notes. Maybe something like this?

Note that using :default with mutually_exclusive will cause multiple parameters to always have a default value and raise a Grape::Exceptions::Validation mutually exclusive exception.

No need to CHANGELOG README changes either, you can remove that. Amend, squash and I'll merge?

@grape-bot
Copy link

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#1720](https://github.com/ruby-grape/grape/pull/1720): Added note for avoid using :default on exactly_one_of - [@gencer](https://github.com/gencer).

Generated by 🚫 danger

@gencer
Copy link
Contributor Author

gencer commented Dec 20, 2017

And you think the right thing. I agree on that.

I amend and squash changes (removed changelog but change the readme)

@dblock dblock merged commit 95484d5 into ruby-grape:master Dec 20, 2017
@dblock
Copy link
Member

dblock commented Dec 20, 2017

Thanks, merged.

@gencer gencer changed the title added note for avoid using :default on exactly_one_of added note for mutually_exclusive params Dec 21, 2017
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.

None yet

3 participants