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

Use required v4 in examples #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonasfj
Copy link

@jonasfj jonasfj commented May 22, 2015

See: #53 (comment)

I still suggest (strongly that we merge the original PR), so people can disable support for required v3-style... and have errors thrown when calling with undefined.

I think it's a solid preparation for a future major release where you fully disable v3-style required.

@navaru
Copy link

navaru commented Aug 18, 2015

Good suggestion, can this be merged?

@mafintosh
Copy link
Owner

What is wrong with the v3 style?

@chrahunt
Copy link
Collaborator

@mafintosh I had suggested it in the context of changes in #53. It makes more sense to have the examples reflect the intended usage. With tools like json-schema-compatibility, it's not necessary to support removed v3 features, which is one less thing to have to worry about in future releases. Your thoughts?

@@ -15,7 +15,7 @@ tape('simple', function(t) {
var validate = validator(schema)

t.ok(validate({hello: 'world'}), 'should be valid')
t.notOk(validate(), 'should be invalid')
t.notOk(validate(null), 'should be invalid')
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we keep the previous one as well? would be nice to test both undefined and null

@LinusU
Copy link
Collaborator

LinusU commented Aug 13, 2018

I think that recommending the latest version is the best, that's the one I've seen most tutorial and other libraries use 👍

@jonasfj sorry for not looking at this earlier, would be happy to merge if you could address my 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.

5 participants