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

Bugfix for issue 699 - scoped mutual methods #774

Merged

Conversation

ShPakvel
Copy link
Contributor

@ShPakvel ShPakvel commented Oct 2, 2014

Hi guys.
I think I figured out how to solve #699 .
Here are changes I've made:

  • change mutually_exclusive to work in scope
  • change exactly_one_of to work in scope
  • change at_least_one_of to work in scope
  • all of those methods changed to work for Hash and Array
  • all of those methods changed to work for requires and optional scope
  • add and change specs

Thank you for the grape!
Pavel

@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 2, 2014

If you need I will add some info to readme as well.

@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 3, 2014

I've added examples to readme.
And add info to changelog file according to your contribution instraction.

@oliverbarnes
Copy link
Contributor

great work cracking the nut :) After a couple of passes it looks solid to me. I'd just change SeveralParamsBase to MultipleParamsBase. Feel a little funny about two base classes, but I can't think of another way really

@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 3, 2014

Hi Oliver!
Thanks for your opinion.
Good point about class name. I will change it.

@@ -1,6 +1,7 @@
0.9.1 (Next)
============

* [#774](https://github.com/intridea/grape/pull/774): Change `mutually_exclusive`, `exactly_one_of`, `at_least_one_of` to work inside any kind of group: `requires` or `optional`, `Hash` or `Array` - [@ShPakvel](https://github.com/ShPakvel).
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: maybe "Enabled mutually_exclusive, ..." or "Extended ..." or "Allowed ...". It's really either a fix or a new feature.

@dblock
Copy link
Member

dblock commented Oct 3, 2014

This is perfect, read the code, great job. Can you squash and I'll merge? Thanks.

def keys_in_common
(attrs.map(&:to_s) & params.stringify_keys.keys).map(&:to_s)
scoped_params.each do |resource_params|
return true if keys_in_common(resource_params).length == 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe .any? works too.

@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 3, 2014

@dblock Great catch regarding any?
I'll change in a minute.

@ShPakvel ShPakvel force-pushed the bugfix_for_699__scoped_mutual_methods branch from d833864 to 732d81e Compare October 3, 2014 21:54

def all_keys
attrs.map(&:to_s)
scoped_params.any? { |resource_params| keys_in_common(resource_params).length < 1 }
Copy link
Member

Choose a reason for hiding this comment

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

This can be .none? I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually here it is not possible. But I can change .length < 1 to .empty?
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh gush, I misunderstood you )) you wanted the same think as me, just another method for this. )) Am I right?

@dblock
Copy link
Member

dblock commented Oct 3, 2014

Rebase too.

…in group

change mutually_exclusive to work in scope
change exactly_one_of to work in scope
change at_least_one_of to work in scope
all of those methods changed to work for Hash and Array
all of those methods changed to work for requires and optional scope
add and change specs
@ShPakvel ShPakvel force-pushed the bugfix_for_699__scoped_mutual_methods branch from 732d81e to f2d586e Compare October 3, 2014 22:26
@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 3, 2014

Check it one more time please. I think this is what you wanted. 😉

@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 3, 2014

@dblock do you need any other changes?

@dblock
Copy link
Member

dblock commented Oct 4, 2014

This is great, merging.

dblock added a commit that referenced this pull request Oct 4, 2014
…ethods

Bugfix for issue 699 - scoped mutual methods
@dblock dblock merged commit 834d06a into ruby-grape:master Oct 4, 2014
@ShPakvel
Copy link
Contributor Author

ShPakvel commented Oct 4, 2014

@dblock I wonder if it is possible to shortly release new gem version with this and other changes/features?
Thanks in advance for your answer.

@dblock
Copy link
Member

dblock commented Oct 4, 2014

Yes. I'll do it soonish.

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