-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Feature] Constant elements in JSON schemas #886
Conversation
Nice! Thank you for the contribution 😄 I assume you moved the constant check to be first in order to avoid generating something else if Also, what kinds of values can constants take on? Maybe a few more test cases for strings, objects, etc. would be good, as well as a test that consts work as expected when used as object properties. @riedgar-ms ping for interest |
Happy to! Yeah, it should come first IMO because Indeed, Thanks for looking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I don't think we need to check that the value in const
matches any type
field; we expect the user to feed us a valid JSON schema. I'm just wondering about negative test cases... although given that all this does is call _to_compact_json()
, I'm not sure there's a huge value there. WDYT @hudson-ai ?
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 56.83% 60.19% +3.35%
==========================================
Files 63 63
Lines 4622 4625 +3
==========================================
+ Hits 2627 2784 +157
+ Misses 1995 1841 -154 ☔ View full report in Codecov by Sentry. |
@wjn0 thank you for the additional tests! @riedgar-ms I agree that checking that type matches the const is unnecessary, as well as extensive negative tests. That being said, I think that there is at least one negative test that would be useful here: If the user passes both a type and a const (presumed to be compatible with each other), we should assert that the const value has "priority". So I think I'd take that last test that has |
@hudson-ai @wjn0 Agree on that extra negative test, to make sure that the |
@hudson-ai @riedgar-ms done! Added a (negative) precedence test, hopefully in keeping with the rest of the suite. |
Awesome!! Looks good to me. |
This is a super quick PR as I work toward supporting some large JSON schemas I'm working with. All it does is support the
const
keyword in JSON schemas, which are a way of hard coding constant elements: https://json-schema.org/understanding-json-schema/reference/const