-
Notifications
You must be signed in to change notification settings - Fork 96
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
Keep inner exceptions if oneOf or anyOf fails #46
Conversation
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.
Looks good in general! Some comments addressed. This also help us on our road to #35
src/Schema/Keywords/OneOf.php
Outdated
foreach ($oneOf as $schema) { | ||
try { | ||
$schemaValidator->validate($data, $schema, $this->dataBreadCrumb); | ||
$matchedCount++; | ||
} catch (SchemaMismatch $e) { | ||
// that did not match... its ok | ||
$innerExceptions[] = $e; | ||
} | ||
} | ||
|
||
if ($matchedCount !== 1) { |
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.
For new exception we should distinguish between zero and >1 matches. For zero matches - use aggregated exception. for >1 we should just provide some useful info, since there is nothing bad with unmatched ones
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 for fast reply. what do you think about this:
- keeping logic like it is for zero matches
- add another exception type "TooManyValidSchemas"* that works like InvalidSchemaCombination but instead of $innerExceptions it stores an array of $validSchemas -> this will be thrown if matched schemas is > 1
*if you have an idea for some better name please tell me.
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.
InvalidSchemaCombination
naming should be changed to NotEnoughValidSchemas
then or something similar, since current naming covers both cases. All the other parts of the idea sound good.
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.
i see, that makes sense. i've updated the code. feel free to check it again :)
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.
LGTM, some minor comments
…s and TooManyValidSchemas exception classes
Thanks @ckdot ! |
this will solve #13