Skip to content

Bugfix for issue 699 - scoped mutual methods#774

Merged
dblock merged 1 commit into
ruby-grape:masterfrom
ShPakvel:bugfix_for_699__scoped_mutual_methods
Oct 4, 2014
Merged

Bugfix for issue 699 - scoped mutual methods#774
dblock merged 1 commit into
ruby-grape:masterfrom
ShPakvel:bugfix_for_699__scoped_mutual_methods

Conversation

@ShPakvel

@ShPakvel ShPakvel commented Oct 2, 2014

Copy link
Copy Markdown
Contributor

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

ShPakvel commented Oct 2, 2014

Copy link
Copy Markdown
Contributor Author

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

@ShPakvel

ShPakvel commented Oct 3, 2014

Copy link
Copy Markdown
Contributor Author

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

@oliverbarnes

Copy link
Copy Markdown
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

ShPakvel commented Oct 3, 2014

Copy link
Copy Markdown
Contributor Author

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

Comment thread CHANGELOG.md Outdated

Copy link
Copy Markdown
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

dblock commented Oct 3, 2014

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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

ShPakvel commented Oct 3, 2014

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

dblock commented Oct 3, 2014

Copy link
Copy Markdown
Member

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

ShPakvel commented Oct 3, 2014

Copy link
Copy Markdown
Contributor Author

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

@ShPakvel

ShPakvel commented Oct 3, 2014

Copy link
Copy Markdown
Contributor Author

@dblock do you need any other changes?

@dblock

dblock commented Oct 4, 2014

Copy link
Copy Markdown
Member

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

ShPakvel commented Oct 4, 2014

Copy link
Copy Markdown
Contributor Author

@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

dblock commented Oct 4, 2014

Copy link
Copy Markdown
Member

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.

3 participants