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

Add mutually_exclusive params validation #626

Merged
merged 1 commit into from
Apr 15, 2014
Merged

Add mutually_exclusive params validation #626

merged 1 commit into from
Apr 15, 2014

Conversation

oliverbarnes
Copy link
Contributor

I'm working on a project using Grape that validates mutually exclusive params quite a bit, and while digging around for an elegant solution, found somebody asking if there was a way to do it through the dsl.

Seems possible to me, thought I'd take a stab at it. Does this work?

ps noob to Grape's code-base, let me know if I'm doing something stupid :)

@oliverbarnes
Copy link
Contributor Author

squashed latest commits with tweaks.

dblock
dblock reviewed Apr 12, 2014
View reviewed changes
lib/grape/locale/en.yml
@@ -28,3 +28,4 @@ en:
unknown_validator: 'unknown validator: %{validator_type}'
unknown_options: 'unknown options: %{options}'
incompatible_option_values: '%{option1}: %{value1} is incompatible with %{option2}: %{value2}'
mutual_exclusion: 'are mutually exclusive params'
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, should this say 'are mutually exclusive', without the '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.

True, removed

@dblock
Copy link
Member

dblock commented Apr 12, 2014

I like it very much.

I know this is internal, but I think the word mutual_exclusion at least sounds weird, should it be mutually_exclusive everywhere?

It needs an update to CHANGELOG and documentation in README, please.

@dblock
Copy link
Member

dblock commented Apr 12, 2014

Another question, how would you do two groups of mutually exclusive parameters?

@oliverbarnes
Copy link
Contributor Author

Awesome, I'm glad you like it.

About mutual_exclusion, not sure. It's definitely a weird-sounding word, but it makes sense to me. I'm following presence in this - required (adjective) params, presence (noun) validator. I do see the other validators follow their keywords (default, regexp), but though they're all validators, in DSL terms it seems like required and optional, are different than default, coerce (which is pretty weird too, try saying it three times). The latter seem incremental to the main validation, options-like. Can't decide whether mutually_exclusive is a high-level thing like required, or more like the others. What do you think?

About different groups of mutually exclusive params, good point - I hadn't thought about it. Seems like in the happy scenario each group should just add another validation error if it's caught, and I've added a spec for this. But it is possible to have conflicting groups. Then again, conflict is possible with a single group too: if someone declares two required params as mutually_exclusive, they'll render the endpoint inaccessible.

I can add a warning to the README noting this. I'll update it and the changelog tomorrow.

Have to say I'm enjoying this DSL design thing ;) It's new to me

dblock
dblock reviewed Apr 14, 2014
View reviewed changes
.rspec
@@ -1,2 +1,2 @@
--color
--format=progress
--format=documentation
Copy link
Member

Choose a reason for hiding this comment

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

Undo this.

@dblock
Copy link
Member

dblock commented Apr 14, 2014

I think multiple groups for multiple mutually exclusive parameters is what we want. Make sure to document that clearly in README.

Squash the commit.

dblock
dblock reviewed Apr 15, 2014
View reviewed changes
CHANGELOG.md
@@ -3,6 +3,7 @@ Next Release

#### Features

* [#626](https://github.com/intridea/grape/pull/626): Mutually exclusive params
Copy link
Member

Choose a reason for hiding this comment

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

Please match the format below, you need to add a - [@username](github link)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@dblock
Copy link
Member

dblock commented Apr 15, 2014

See my minor comments. It's ready to merge otherwise.

@oliverbarnes
Copy link
Contributor Author

Done.

dblock added a commit that referenced this pull request Apr 15, 2014
@dblock
Merge pull request #626 from oliverbarnes/mutually_exclusive
1e1b1f7 
Add mutually_exclusive params validation
@dblock dblock merged commit 1e1b1f7 into ruby-grape:master Apr 15, 2014
@dblock
Copy link
Member

dblock commented Apr 15, 2014

Merged, thank you.

@oliverbarnes oliverbarnes deleted the mutually_exclusive branch April 15, 2014 17:58
@dm1try dm1try mentioned this pull request Apr 25, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oliverbarnes @dblock