Skip to content

Conversation

@PhilippeChab
Copy link

See #657 (comment) for the reason behind this PR.

This solution is not the cleanest but it actually give us a way to have a decent model name in the documentation. It can be temporary.

I didn't read the source code all over, but I don't understand why the params key doesn't have the same behavior as the success one. I think it could be a permanent solution.

@coveralls
Copy link

coveralls commented Feb 21, 2020

Coverage Status

Coverage increased (+0.0006%) to 99.451% when pulling 6b0cbf0 on PhilippeChab:add-params-model-name-in-route-options into fada256 on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.453% when pulling 6b0cbf0 on PhilippeChab:add-params-model-name-in-route-options into fada256 on ruby-grape:master.

@PhilippeChab
Copy link
Author

PhilippeChab commented Feb 24, 2020

I've experimented more with grape, grape-swagger and grape-entity and I just don't understand the way it's designed at the moment.

Why would you want to add a params key in your desc block like this:

desc: "Some endpoint",
    params: API::V1::Entities::SomeEntity,
    ...
get ":id" do
...

since you can already add documentation to your params with grape this way:

params do
    requires :some_param, type: String, documentation: { desc: "Some description", in: "body" }
    ...
get ":id" do
...

Because encapsulating the params into an entity is a cleaner way to do this? Yes, I agree. But the problem is, the params in the desc: ... block are not validated, so you absolutely need to add the route params in the params block. What is the point to document the params in both block then? I don't see any reason, which would mean the params key in the desc block is useless. If there is something I am missing, please tell.

That being said, I still think entities are a really nice facade to endpoint responses and I don't understand why they can't be use directly in grape params block for endpoint params. Why can't we do something like this:

des "...",
    ....
params API::V1::Entities::SomeEntity
get ":id" do
    ...

This would make an entity really great, since it could provide validation, documentation and encapsulation all at once.

I'm closing this PR since I don't see any reason to integrate this temporary fix.

EDIT:
This should be the way the params key in the desc block should be working if a documentation block should ever support validation: https://stackoverflow.com/questions/29517362/grape-required-params-with-grape-entity

@PhilippeChab PhilippeChab changed the title Add params model in route options Add params model name in route options Feb 24, 2020
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.

2 participants