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

feat: dependent required tag #303

Merged

Conversation

victoraugustolls
Copy link
Sponsor Contributor

This pull request adds the ability to define some fields to be required only when other fields are present with the struct tag dependentRequired.

Resolves issue #296

@victoraugustolls
Copy link
Sponsor Contributor Author

@danielgtaylor do you think we should write a how-to docs for conditional-fields?

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.11%. Comparing base (41e6941) to head (5b2b9c3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
+ Coverage   95.08%   95.11%   +0.03%     
==========================================
  Files          19       19              
  Lines        2744     2765      +21     
==========================================
+ Hits         2609     2630      +21     
  Misses         98       98              
  Partials       37       37              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@victoraugustolls
Copy link
Sponsor Contributor Author

victoraugustolls commented Mar 13, 2024

@danielgtaylor While making this pull request, there was a schema.Required and schema.requiredMap, so I followed the same approach for the dependentRequired. But we could get away only with the DependentRequired if that is okay with you. I will push the changes. Let me know if you'd like me to revert them.

One thing to notice: I did not validate if the dependents fields exist, but I could add this validation and panic, as it seems there is no way to return an error. Since this would panic when creating the schema, it wouldn't impact a running app. Let me know your thoughts on this.

Copy link
Owner

@danielgtaylor danielgtaylor left a comment

Choose a reason for hiding this comment

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

LGTM, great work! Thank you 👍

I think it would be good to panic if there is a typo or the wrong field name is provided, so I'll hold off merging unless you want to do that in another PR.

@danielgtaylor
Copy link
Owner

Oh, and one last thing, you'll want to modify https://github.com/danielgtaylor/huma/blob/main/docs/mkdocs.yml#L54-L59 so your new how-to doc shows up in the menu properly!

@victoraugustolls
Copy link
Sponsor Contributor Author

@danielgtaylor done! added the panic for non existing fields and updated the docs!

@danielgtaylor danielgtaylor merged commit 771a002 into danielgtaylor:main Mar 13, 2024
3 checks passed
@victoraugustolls victoraugustolls deleted the feat/dependent-required branch March 13, 2024 15:39
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.

None yet

2 participants