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

Fix handling of entity with class names without "Entity" or "Entities" #75

Merged
merged 7 commits into from
Jan 30, 2025

Conversation

numbata
Copy link
Contributor

@numbata numbata commented Jan 28, 2025

Improves the handling of unconventional entity class names and enhances support for diverse types in Swagger documentation generation.

Changes

  • Fixed ambiguous_model_type? to correctly identify entities with non-standard class names.
  • Introduced primitive_type? for clearer handling of primitives and additional non-class types (string, array).
  • Enabled support for custom type parsers to handle registered types for parsing.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks great. Can I OCD over a nit please? Sorry!

@@ -191,6 +201,7 @@ class SomeEntity < Grape::Entity
expose :nested, using: TheseApi::Entities::Nested, documentation: { desc: 'Nested object.' }
expose :nested_child, using: TheseApi::Entities::NestedChild, documentation: { desc: 'Nested child object.' }
expose :polymorphic, using: TheseApi::Entities::Polymorphic, documentation: { desc: 'Polymorphic Model' }
expose :mixed, using: TheseApi::Entities::MixedType, documentation: { desc: 'A model with mix of types' }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expose :mixed, using: TheseApi::Entities::MixedType, documentation: { desc: 'A model with mix of types' }
expose :mixed, using: TheseApi::Entities::MixedType, documentation: { desc: 'A model with mix of types.' }

(and let's replace Polymorphic Model with A polymorphic model. above, to be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done here: 47e6140

@numbata
Copy link
Contributor Author

numbata commented Jan 28, 2025

Can I OCD over a nit please?

Of course! All suggestions are always welcome.

@@ -30,7 +30,7 @@ jobs:
strategy:
fail-fast: false
matrix:
ruby-version: ['3.0', '3.1', '3.2', '3.3']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm.. Maybe instead of removing ruby 3.0, the specific grape version can be set instead. As a separate PR ofc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea, will merge this one, feel free to pick it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will do this.

@dblock dblock merged commit 0c1af79 into ruby-grape:master Jan 30, 2025
5 checks passed
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