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

Add support for JSON schema #58

Merged
merged 12 commits into from
Apr 2, 2019
Merged

Add support for JSON schema #58

merged 12 commits into from
Apr 2, 2019

Conversation

yaodingyd
Copy link
Contributor

Fix sindresorhus/electron-store#16

Use epoberezkin/ajv for JSON schema validation

@sindresorhus
Copy link
Owner

You need to consider how the defaults option will work with schema defaults. There's overlap. https://github.com/epoberezkin/ajv#assigning-defaults

@sindresorhus
Copy link
Owner

This PR is a bit thin considering the expected work for the bounty.

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 11, 2019

The readme documentation should have a full realistic example of how it should be used. Meaning, various kinds of types and validations options.

@sindresorhus
Copy link
Owner

Also go through all the options in https://github.com/epoberezkin/ajv#validation-and-reporting-options and carefully consider which one to use or not.

@sindresorhus
Copy link
Owner

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@yaodingyd yaodingyd changed the title Add support for JSON schema Add support for JSON schema(WIP) Feb 12, 2019
@yaodingyd
Copy link
Contributor Author

I go through all the options for ajv:

  1. We would only use basic and standard type and formats, no $data references, no custom formats;
  2. We don't use referenced schema options(schemaId, missingRefs, extendRefs or loadSchema);
  3. Except to useDefault, we don't want to modify validated data(removeAdditional or coerceTypes);
  4. We don't use any advanced options except errorDataPath, which is good for reporting errors.
    So I think to use the same options makes sense.

@yaodingyd yaodingyd changed the title Add support for JSON schema(WIP) Add support for JSON schema Feb 22, 2019
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
sindresorhus and others added 3 commits February 27, 2019 08:50
Co-Authored-By: yaodingyd <yaodingyd@gmail.com>
@yaodingyd
Copy link
Contributor Author

I was wrong about the overwrite order before: actually we apply Conf's defaults first, thus schema's default would not be used because value has already be set. I have updated doc accordingly.

readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus merged commit 373afb9 into sindresorhus:master Apr 2, 2019
@sindresorhus
Copy link
Owner

Looks good now :)

@yaodingyd yaodingyd deleted the schema branch April 2, 2019 12:51
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.

2 participants