Skip to content

Make entity fields required by default #81

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

olivier-thatch
Copy link

@olivier-thatch olivier-thatch commented May 7, 2025

This PR changes how grape-swagger-entity determines if an entity field is required or not:

  1. if documentation: { required: true/false } is set, use that
  2. otherwise, if if or unless is not null or if expose_nil is false, assume the parameter is not required
  3. otherwise, assume the parameter is required

This is consistent with how grape-entity renders fields: fields are always exposed unless if or unless is provided (cf. https://github.com/ruby-grape/grape-entity?tab=readme-ov-file#conditional-exposure).

Closes #80.

This is probably a breaking change, so I understand if you don't want to merge this as is, but it would be nice to be able to opt into this behavior.

@numbata numbata self-requested a review May 7, 2025 10:45
Copy link
Collaborator

@numbata numbata left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution, @olivier-thatch! The changes look generally good, and I appreciate that you not only highlighted this problem, but also suggested a solution. I do have some thoughts on a potential simplification in one of the methods, which I've detailed in a comment below.

@olivier-thatch olivier-thatch force-pushed the required-by-default branch from 0a992e3 to 14455d2 Compare May 8, 2025 00:23
@olivier-thatch olivier-thatch requested a review from numbata May 8, 2025 02:56
@olivier-thatch olivier-thatch force-pushed the required-by-default branch from 14455d2 to e017856 Compare May 8, 2025 20:40
@olivier-thatch olivier-thatch requested a review from numbata May 8, 2025 20:42
@olivier-thatch olivier-thatch force-pushed the required-by-default branch 3 times, most recently from 07a3f3b to 374be93 Compare May 12, 2025 21:33
@olivier-thatch
Copy link
Author

I've updated the PR to account for another case of conditional exposure: expose_nil: false (cf. docs).

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.

Make entity fields required by default
2 participants