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

Proposal for shared params #735

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dspaeth-faber
Copy link
Contributor

Like discussed in #716 I would suggest this DSL for shared params instead of using Helpers.

@dblock dblock mentioned this pull request Aug 25, 2014
@dblock
Copy link
Member

dblock commented Aug 25, 2014

I am not in love with the name include_params. It seems that you would want to use all kinds of things, params being one of them. I'd like to be able to write use pagination.

I also don't love the SharedParams name, I think maybe it should just be Params?

@dspaeth-faber
Copy link
Contributor Author

Yeah, it's only a starting point for discussion. Without seeing some code it's difficult some times to discuss things. include_params is only needed because all of the imbue thing like it works today.

In theorie it should be possible to use use inside the params block only. But then it would not be possible to use the same name multiple times within different scopes.

@dm1try
Copy link
Member

dm1try commented Aug 26, 2014

@dspaeth-faber, @dblock which problem does PR solve? How do you use a "shared params" feature?

from here

In current implementation helper module can include "Enpoint-Instance" and "API-Class" helpers through single syntax helpers.

@dspaeth-faber
Copy link
Contributor Author

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.

shared_params provide a global registry for shared parameters and make it sematicaly clear what is meant.

Furthor more you can extend your module from Grape::SharedParams and use this module together with include_params. This makes it more obvoius what is happening.

Using helpers ParamsModule seem's not obvious to me. That some thing is not ok can you see within the original Helpers module. The methods within this module have only one purpose provide access to the API-Class settings to inject new parameters and they come into play during "compile time". Other DSL methods from API class level are not available.

In my point of view there should a distinction between Helper modules and other DSL methods.
Helpers are the only way to provide Methods to your Endpoint instance and this should be all their
responsibilities.

@dm1try
Copy link
Member

dm1try commented Sep 11, 2014

@dspaeth-faber fair enough:ok_hand:

@dblock
Copy link
Member

dblock commented Dec 30, 2014

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'
Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

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

@dmitry good point 👍

@dm1try
Copy link
Member

dm1try commented Nov 29, 2016

So @dblock there are no other opinions :)
I can push this forward and finish the PR if needed.

@dblock
Copy link
Member

dblock commented Nov 30, 2016

Thanks @dm1try

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.

4 participants