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

Added {useDefault: true} option flag to substituted in default values #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonasfj
Copy link

@jonasfj jonasfj commented Apr 13, 2015

Fixing issues #48.

Basically the useDefault option flag will allow substitution of default values from JSON schema.
These will nest and they will not work for required properties. Validation will also validate the default value. Ideally we could do this at compile-time.

It works by mutating the input, so it only works if top-level value is an object or array. But this seems like a reasonable limitation. It's very very nice that we get default substituted in along with validation.
Proper warnings about this being a non-standard feature and it being mutating the input object have been added to README.md.


I generally agree that features like useDefault and filter are dangerous. And they should definitely be hidden behind feature flags. But hiding them in methods like validator.filter seems to make little sense, one of the primary features is this being fast and getting the validation done along with default value substitution. + the ability to combine filter and useDefault.

Note, I understand anyone having mixed feelings about adding non-standard features like this. But in practice they are very useful and they have no performance overhead as they are enabled/disabled at compile-time.

if (node.default !== undefined && opts && opts.setDefault === true) {
validate
('if (%s === undefined) {', name)
('console.log(%s)', JSON.stringify(name))
Copy link
Owner

Choose a reason for hiding this comment

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

could you remove the log?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that console.log... Pretty sad that I managed to somehow slip that into a 6 line patch :)
After all everything else is just tests and docs.

@jonasfj jonasfj force-pushed the default-value-substitution branch from 1c05bf9 to c5629d2 Compare May 11, 2015 23:50
@calebmer
Copy link

calebmer commented Jul 3, 2015

Could this please be merged?

@RangerMauve
Copy link
Contributor

This'd be nice

@jonasfj
Copy link
Author

jonasfj commented Dec 18, 2015

Note, this PR has some limitation with respect to anyOf, etc, see discussion here about the complexities of doing this right: ajv-validator/ajv#42 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants