-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Proposal for shared params #735
base: master
Are you sure you want to change the base?
Conversation
I am not in love with the name I also don't love the |
Yeah, it's only a starting point for discussion. Without seeing some code it's difficult some times to discuss things. In theorie it should be possible to use |
@dspaeth-faber, @dblock which problem does PR solve? How do you use a "shared params" feature? from here
|
This PR is porposal to solve the semantic gap. Helpers extend an Enpoint instance. Params work on API-Class level. This seems like a missmatch.
Furthor more you can extend your module from Using In my point of view there should a distinction between Helper modules and other DSL methods. |
@dspaeth-faber fair enough:ok_hand: |
I am still 50/50 about this. I understand your point @dspaeth-faber, it makes sense. But something doesn't feel right here. I would like more opinions. |
end | ||
end | ||
|
||
include_params Grape::API::HelpersSpec::SharedParams, 'a name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there are include_params
required? Why not simply:
shared_params :pagination_more do
optional :per_page, type: Integer
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmitry good point 👍
So @dblock there are no other opinions :) |
Thanks @dm1try |
Like discussed in #716 I would suggest this DSL for shared params instead of using Helpers.