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

Optional groups #443

Merged
merged 10 commits into from
Aug 6, 2013
Merged

Optional groups #443

merged 10 commits into from
Aug 6, 2013

Conversation

asross
Copy link
Contributor

@asross asross commented Jul 19, 2013

See issue #319.

Might be better to go with `optional_group(element)` than `group(element, required: false)`?
@dblock
Copy link
Member

dblock commented Jul 22, 2013

Thanks.

I smell too many parameters in ParamsScope. Maybe it's time for it to take a hash of options?

Could you please update CHANGELOG, too.

@asross
Copy link
Contributor Author

asross commented Jul 22, 2013

Agreed about ParamsScope. Not sure if you want that done for this pull request; I'll try to take a pass at it later if I have time. Done.

@asross
Copy link
Contributor Author

asross commented Jul 22, 2013

Also, this would be a breaking change, but it might be simpler to do:

optional :group1 do
  requires :field1
end

requires :group2 do
  requires :field2
end

That is, instead of having methods group and optional_group, just allow optional and requires to take blocks that instantiate new scopes.

@dblock
Copy link
Member

dblock commented Jul 22, 2013

You can probably introduce optional and requires the way you suggest without removing group and optional_group, that would be backward compatible, right?

There's a merge conflict, could you merge from upstream please.

@@ -3,6 +3,7 @@ Next Release
#### Features

* Grape is no longer tested against Ruby 1.8.7.
* [#443](https://github.com/intridea/grape/pull/443): Added `optional_group` to ParamsScope to let users validate the internal structure of a group without requiring its presence. - [@asross](https://github.com/asross).
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra period before your name - [ ... :)

Aside of that HUGE deal (j/k), maybe something simpler as a one-liner, like the fact that you added optional_group that does X?

@dblock
Copy link
Member

dblock commented Jul 22, 2013

Given your last comment I wonder whether optional_group is the best choice. What if it was optional to begin with and group and requires were aliases of each-other?

@asross
Copy link
Contributor Author

asross commented Jul 22, 2013

Yeah, I'm thinking similarly, although I don't think you should be able to call group without a block (it could just call requires, though). Best not to introduce the optional_group method only to have it immediately become deprecated.

@asross
Copy link
Contributor Author

asross commented Jul 22, 2013

What do you think of that change? The only thing I feel leery about is how to handle extra arguments to requires and optional when a block is given. I decided to raise an argument error here, but maybe there's a better solution.

@dblock
Copy link
Member

dblock commented Jul 22, 2013

Love it. I am going to let it maroon here for a day or so and will merge unless I hear objections from other people.

dblock added a commit that referenced this pull request Aug 6, 2013
@dblock dblock merged commit 01f3b81 into ruby-grape:master Aug 6, 2013
@dblock
Copy link
Member

dblock commented Aug 6, 2013

I merged this, thank you so much. Good work.

@asross
Copy link
Contributor Author

asross commented Aug 7, 2013

Thanks! Glad to be of help.

@zuk
Copy link

zuk commented Sep 9, 2013

Thanks for this!

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