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

Update to latest fast-json-patch #23

Merged
merged 2 commits into from
Jun 25, 2018

Conversation

donaldjarmstrong
Copy link
Contributor

  • fix the warning 'jsonpatch.apply is deprecated, please use applyPatch for applying patch sequences, or applyOperation to apply individual operations.'
  • Update to 2.0.6 version of fast-json-patch and address the deprecation warning
  • Update tests to include patch array of operations
  • Update docs with an example reason to use patch with a link to the json-patch spec

* fix the warning 'jsonpatch.apply is deprecated, please use `applyPatch` for applying patch sequences, or `applyOperation` to apply individual operations.'
* Update to 2.0.6 version of fast-json-patch and address the deprecation warning
* Update tests to include patch array of operations
* Update docs with an example reason to use patch with a link to the json-patch spec
@donaldjarmstrong
Copy link
Contributor Author

Also address #12

@coveralls
Copy link

coveralls commented Mar 2, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 5708ba9 on donaldjarmstrong:master into bc61807 on epoberezkin:master.

@epoberezkin
Copy link
Member

Will review :)


module.exports = function(ajv) {
addKeyword(ajv, '$patch', jsonPatch, {
addKeyword(ajv, '$patch', jsonPatch, 'applyPatch', {
Copy link
Member

Choose a reason for hiding this comment

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

you can pass jsonPatch.applyPatch here and jsonMergePatch.apply above. Then in add_keyword you would just need to use jsonPatch(null, source, patch, true); without any additional parameters.

},
"with": [
{ "op": "add", "path": "/properties/q", "value": { "type": "number" } }
{ "op": "add", "path": "/properties/q", "value": { "type": "number" } },
{ "op": "add", "path": "/required/-", "value": "q" }
Copy link
Member

Choose a reason for hiding this comment

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

if you want to add "required" keyword and its change with "patch", then it has to be added with "merge" as well (it can replace the whole array) and also tested in "test_validate.js" (object without "q" should fail); as is it is not tested at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the review. Changes submitted

* remove extra parameter from add_keyword; instead change the invoke targets to pass function
* make the tests all require 'q' property; confirm it in test_validate.
@epoberezkin epoberezkin merged commit f1e8831 into ajv-validator:master Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants