Closed wjn0 closed 3 months ago
Nice! Thank you for the contribution :smile:
I assume you moved the constant check to be first in order to avoid generating something else if type
is specified? Would you mind adding a test to make sure that behavior is working as expected?
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 const
s can optionally be typed, e.g. {"type": "integer", "const": 1}
is a valid schema (however, if 1
was instead "1"
that would be an invalid schema, so we don't need to do any type checking once there is a const
). I added a test to that effect.
Indeed, const
s can be anything, so I added a test for each of several types (integers, strings, arrays, and complex objects), along with a test for when a const
is a property of an object.
Thanks for looking!
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 60.19%. Comparing base (
325b2c8
) to head (1d0547d
).
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@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 {"type": "integer", "const": 1}
and turn it into a negative test that asserts that 2
doesn't match, for example. What do you two think?
@hudson-ai @wjn0 Agree on that extra negative test, to make sure that the const
takes precedence over any type
.
@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