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

Remove unused methods in GrapeSwagger::DocMethods::BuildModelDefinition #856

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

takahashim
Copy link
Contributor

I was reading the code for GrapeSwagger::DocMethods::BuildModelDefinition and noticed that parse_entity() and parse_representable() return only nil.
Then, since required_attributes() also returns only nil, the if statement in build() would also be unnecessary since required_attrs.blank? would always be true.

Based on this code reading, I've tried to remove the unused code.

@LeFnord
Copy link
Member

LeFnord commented Apr 1, 2022

mmh, a bit curious … do you checked it with model-parsers?

@takahashim
Copy link
Contributor Author

Thanks for your comment. Yes, it is curious indeed.

It is a bit late, but I looked at the code and issues/PRs of grape-swagger-entity and grape-swagger-representable.
The trigger for this issue seemed to be a issue Missing required attributes from merged entities and a pull request Required properties #696 to resolve it.

In #696, required is added to the return value when call-ing model_parsers, and passed it to GrapeSwagger::DocMethods::BuildModelDefinition.build().
Then, if required is nil, while falling back to the original process, a deprecated warning is output in parse_entity or parse_representable.

The deprecated warning added here is then removed in #768.

It is no problem that deprecated_workflow_for is removed in #768, but the return unless line above it should have been removed as well. The reason is that this line was intended to skip the subsequent warning and processing.
As a result, parse_entity and parse_representable are now methods that do nothing. And the required_attributes method that calls these two methods could have been removed as well.

@LeFnord
Copy link
Member

LeFnord commented Jul 26, 2022

thanks @takahashim … good spot

besides I like DDD (delete driven desing) … 😉

@LeFnord LeFnord merged commit d576663 into ruby-grape:master Jul 26, 2022
aka-momo pushed a commit to aka-momo/grape-swagger that referenced this pull request Feb 8, 2023
…on (ruby-grape#856)

Co-authored-by: peter scholz <pscholz.le@gmail.com>
Bhacaz pushed a commit to Bhacaz/grape-swagger that referenced this pull request Aug 31, 2023
…on (ruby-grape#856)

Co-authored-by: peter scholz <pscholz.le@gmail.com>
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