Skip to content

Don't overwrite model description with the route description. #804

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

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

Bhacaz
Copy link
Contributor

@Bhacaz Bhacaz commented Jul 28, 2020

This PR have 3 features around the API response documentation.

  1. Removing the assignation of the model description from route description. Some work was already started in this PR Corrected error in description of Models (definitions) #629.
  2. Generate models documentation before the routes.
  3. Add the ability to specified no model (or empty body).

1. Removing the assignation of the model description from route description.

I face the same problem explain in:

And I try to continued to resolved it.

Following the PR comments and the bug suggestion, the change in this PR will set a description to the model only if it doesn't have one and with a generic sentence.

2. Generate models documentation before the routes.

When I was generating my documentation I observe that sometimes Grape::Endpoint#response_object try to find the model definition without succeed with Grape::Endpoint#expose_params_from_model simply because the route documentation was generated before the model definition.

3. Add the ability to specified no model (or empty body).

The last thing I add was the ability explicitly set no model (or an empty body) in the response, for example in the failures. The problem was that when none is explicitly passed, Grape::Endpoint#response_object is really good to found a model from the path and add it to the documentation.

I hope it will be satisfying.

@grape-bot
Copy link

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [###](https://github.com/ruby-grape/grape-swagger/pull): Don't overwrite model definition with the route description. - [@Bhacaz](https://github.com/Bhacaz)

Generated by 🚫 danger

3 similar comments
@grape-bot
Copy link

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [###](https://github.com/ruby-grape/grape-swagger/pull): Don't overwrite model definition with the route description. - [@Bhacaz](https://github.com/Bhacaz)

Generated by 🚫 danger

@grape-bot
Copy link

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [###](https://github.com/ruby-grape/grape-swagger/pull): Don't overwrite model definition with the route description. - [@Bhacaz](https://github.com/Bhacaz)

Generated by 🚫 danger

@grape-bot
Copy link

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [###](https://github.com/ruby-grape/grape-swagger/pull): Don't overwrite model definition with the route description. - [@Bhacaz](https://github.com/Bhacaz)

Generated by 🚫 danger

@grape-bot
Copy link

grape-bot commented Jul 28, 2020

1 Error
🚫 One of the lines below found in CHANGELOG.md doesn’t match the expected format. Please make it look like the other lines, pay attention to periods and spaces.
* [#804](https://github.com/ruby-grape/grape-swagger/pull/804): Don't overwrite model description with the route description. - [@Bhacaz](https://github.com/Bhacaz)

Generated by 🚫 danger

@Bhacaz Bhacaz force-pushed the rework_on_response_object branch 2 times, most recently from b200802 to a828ce0 Compare July 28, 2020 20:07
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.473% when pulling f6ac1b2 on Bhacaz:rework_on_response_object into 588579d on ruby-grape:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.473% when pulling f6ac1b2 on Bhacaz:rework_on_response_object into 588579d on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 99.473% when pulling f6ac1b2 on Bhacaz:rework_on_response_object into 588579d on ruby-grape:master.

@coveralls
Copy link

coveralls commented Jul 28, 2020

Coverage Status

Coverage remained the same at 99.47% when pulling 338513f on Bhacaz:rework_on_response_object into 588579d on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Sep 2, 2020

yeap that does it 😄 … thanks @Bhacaz

please can you add an entry in UPGRADING.md with title: Upgrading to >= 1.3.0, thanks

@Bhacaz
Copy link
Contributor Author

Bhacaz commented Sep 3, 2020

please can you add an entry in UPGRADING.md with title: Upgrading to >= 1.3.0

Sure!

@Bhacaz Bhacaz force-pushed the rework_on_response_object branch 3 times, most recently from c6cef6a to fc7b5b1 Compare September 3, 2020 15:42
@Bhacaz Bhacaz force-pushed the rework_on_response_object branch from fc7b5b1 to 338513f Compare September 3, 2020 15:50
@Bhacaz
Copy link
Contributor Author

Bhacaz commented Sep 3, 2020

I needed to refactor lib/grape-swagger/doc_methods.rb to satisfy Rubocop (0.90.0)

@Bhacaz Bhacaz requested a review from LeFnord September 3, 2020 16:34
next if value[:code] == 204
next unless !response_model.start_with?('Swagger_doc') && (@definitions[response_model] || value[:model])
# Explicitly request no model with { model: '' }
next if value[:model] == ''
Copy link
Member

Choose a reason for hiding this comment

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

mmh … ok … will have a look on it later, maybe it can be get together with no model key given

Copy link
Contributor Author

@Bhacaz Bhacaz Sep 8, 2020

Choose a reason for hiding this comment

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

@LeFnord I did like that because by default if no model key is provided (line 219) the model use for the response will be the one returned by GrapeSwagger::DocMethods::PathString#build called here

@item, path = GrapeSwagger::DocMethods::PathString.build(route, options)

@LeFnord
Copy link
Member

LeFnord commented Sep 3, 2020

very good job, many thanks @Bhacaz

@LeFnord LeFnord merged commit 565f8e9 into ruby-grape:master Sep 3, 2020
@Bhacaz Bhacaz deleted the rework_on_response_object branch September 8, 2020 14:03
aka-momo pushed a commit to aka-momo/grape-swagger that referenced this pull request Feb 8, 2023
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