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 if-then-else, anyOf, dependencies #197

Merged

Conversation

karenetheridge
Copy link
Contributor

Summary

fixes bugs: #190, #196, #192 where various keywords were not fully supported

Motivation

see their respective issues for background and references

I am not quite sure what error messages are best: "Missing property" was already being used by the 'required' keyword, and it's not great to have two different errors sharing the same message, so I made the 'required' error "Missing required property", so 'dependencies' can have "Missing dependent property". However, changing an existing error message might break unit tests in downstream code.

Copy link
Owner

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

I have a hard time accepting this PR, since there's so many unrelated changes bundled into one PR. I wish it was split into three PR's. Either way, I want to merge it 👍

Please review my comments.

lib/JSON/Validator/Error.pm Outdated Show resolved Hide resolved
lib/JSON/Validator.pm Outdated Show resolved Hide resolved
push @errors,
$self->_validate_any_of($to_json ? $$to_json : $_[1], $path, $rules);
}
elsif ($rules = $schema->{oneOf}) {

if (my $rules = $schema->{oneOf}) {
Copy link
Owner

Choose a reason for hiding this comment

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

Side note: I really hope nobody actually uses this. Seems like a too complicated schema to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a combination of oneOf, anyOf and allOf all at the same time? hey, you never know :D

lib/JSON/Validator.pm Outdated Show resolved Hide resolved
push @errors,
$self->_validate_one_of($to_json ? $$to_json : $_[1], $path, $rules);
}

if ($schema->{if}) {
Copy link
Owner

Choose a reason for hiding this comment

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

👍

lib/JSON/Validator.pm Outdated Show resolved Hide resolved
lib/JSON/Validator/Error.pm Outdated Show resolved Hide resolved
@karenetheridge karenetheridge force-pushed the ether/if-anyOf-dependencies branch 2 times, most recently from 063dce5 to 3c314d9 Compare February 14, 2020 22:54
lib/JSON/Validator.pm Show resolved Hide resolved
@@ -35,6 +35,7 @@ our $MESSAGES = {
maxProperties => 'Too many properties: %3/%4.',
minProperties => 'Not enough properties: %3/%4.',
required => 'Missing property.',
dependencies => 'Missing property: Dependee: %3.',
Copy link
Owner

Choose a reason for hiding this comment

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

Too many colons. How about "Missing property. Dependee: %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.

fixed.

@karenetheridge karenetheridge force-pushed the ether/if-anyOf-dependencies branch 2 times, most recently from 8b5ab16 to e759a15 Compare February 14, 2020 23:15
@jhthorsen
Copy link
Owner

Awesome 👍

@jhthorsen jhthorsen merged commit 154739f into jhthorsen:master Feb 14, 2020
jhthorsen pushed a commit that referenced this pull request Feb 14, 2020
 - Add support for "dependencies" keyword #192 #197
 - Add support for anyOf/allOf/oneOf at the same time #196 #197
 - Allow if/then/else to be in any sort of schema #190 #197
@karenetheridge karenetheridge deleted the ether/if-anyOf-dependencies branch February 14, 2020 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants