WIP: add format support to numeric types #350
Conversation
| @@ -2,6 +2,7 @@ | |||
| {{ | |||
| var $lvl = it.level; | |||
| var $dataLvl = it.dataLevel; | |||
| var $type = it.schema.type || ''; | |||
There was a problem hiding this comment.
This could be an array according to JSON schema spec?
There was a problem hiding this comment.
yes, can be any of basic types or integer
| ajv.addFormat('identifier', /^[a-z_$][a-z0-9_$]*$/i); | ||
| testFormat(); | ||
| }); | ||
| describe.skip('without any type restrictions specified', function() { |
There was a problem hiding this comment.
I think a reasonable approach to this is: If format is not explicitly declared in the schema then custom formatters should not be applied?
There was a problem hiding this comment.
I don't understand. If you mean that "if type is not specified", then it is not correct. Format should be applied string and number after this change, depending on format keyword value, but even if the type is not explicitly specified. So this schema { "format": "int32" } would be valid for numbers only if they are int32 but all other types will be valid too. You need to think about keywords as about restrictions. Type keyword is also a restriction, but it does not affect format validation, data type does.
There was a problem hiding this comment.
By the way, rules are called based on the data type, not on the presence of type keyword. So the way you did it (format keyword added to two types) there can be some code added for both string and integer data type and the code in format.jst should skip adding code if type is incorrect (I think you're doing it already, and I suggested to simplify not to support arrays of types). So I don't think that type keyword should be explicitly specified for it to work.
see comment above
I think there should be a way to define format for any types. Type that the format applies to should be a part of format definition with string being the default (if type is not present in the definition). EDIT: actually it's ok, only for string and number is fine, there is no use case really to use format for arrays/objects/boolean/null, other keywords should be used for them. |
| @@ -81,11 +81,15 @@ | |||
| var $isObject = typeof $format == 'object' | |||
| && !($format instanceof RegExp) | |||
| && $format.validate; | |||
| var $types = $isObject && $format.types || ['string']; | |||
| if ($isObject) { | |||
There was a problem hiding this comment.
Let's not support formats that apply to multiple types, there is no real use case for it and it would make code simpler.
|
Thank you, it's getting there it seems :) Some more tests and docs and it's done. Also, I am going to merge it into 5.0.0 branch - I will release the next beta version once it's finished. |
|
@nimerritt finished and merged in 5.0.0 |
|
Thanks for finishing this up for me :) Actually needed it again today, so 👍 |
PR: for discussion
See #291 for context.