-
Notifications
You must be signed in to change notification settings - Fork 423
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/improve error checks for gltf #119
Conversation
17164ea
to
5361546
Compare
@@ -111,10 +111,6 @@ public JsonSchemaValidationException Validate<T>(JsonSchemaValidationContext c, | |||
} | |||
|
|||
var value = o as string; | |||
if (value.All(x => Char.IsWhiteSpace(x))) |
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.
json schema allows empty string
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.
null check ?
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'll add null checks for all validators.
} | ||
|
||
return null; | ||
return GenericValidator<T>.Validate(Required, Properties, c, o); |
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.
Check for all fields not only required one.
@@ -47,7 +47,8 @@ public static void SerializeObject(this IFormatter f, object value) | |||
} | |||
else | |||
{ | |||
typeof(FormatterExtensionsSerializer).GetMethod("Serialize").MakeGenericMethod(value.GetType()).Invoke(null, new object[] { f, value }); | |||
typeof(FormatterExtensionsSerializer).GetMethod("Serialize") | |||
.MakeGenericMethod(value.GetType()).Invoke(null, new object[] { f, value }); |
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.
Only add a newline.
@@ -92,7 +93,7 @@ static class GenericSerializer<T> | |||
var mi = typeof(IFormatter).GetMethod("Value", new Type[] { t }); | |||
if (mi != null) | |||
{ | |||
// premitives | |||
// primitives |
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.
typo
return null; // There are no json schema for items, success | ||
} | ||
|
||
var v = Items.Validator; |
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.
Check items if there is a validator.
Please write these exceptions to VRM specification document. When finished, I apply these changes to VRM Validator . |
…e checks for validations
…rray/List. Can pass JsonSchemaValidationContext to Serializer optionally
5361546
to
579d533
Compare
Thank you for your comment. I think that json schema is not affected by this changes. Types of Json schema, string and list( NOTE: Current implementation exclude fields which have empty/containing only whitespaces string and 0-length list. |
GLTF spec requires an item at least per array fields (see below) and it was an improvement of the schema between 1.0 and 2.0 (see below) GLTF 1.0: GLTF 2.0: I know we are not bound to this rule but I think we should follow this way. |
Yes, you are right. Your concerns are cleared. Fields which have constraints like |
Applying changes for glTF/VRM schames will be done by another PR. |
Changes for glTF/VRM: #123 |
EnableDiagnosisForNotRequiredFields = true
ExplicitIgnorableValue
attributesnull
is still disallowed""
and" "
are allowed nowpattern
ormin/maxLength
insteadnull
is still disallowed[]
is allowed nowmin/maxItems
instead