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

Test the removal of the whole document #29

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

Conversation

espadrine
Copy link

@sonnyp
Copy link

sonnyp commented Aug 24, 2016

replacing the document with null is not removing the document

I believe the behavior of removing / should be left to the implementation as it cannot be semantically defined by json patch

In https://github.com/JSON8/patch I return undefined

@espadrine
Copy link
Author

espadrine commented Aug 24, 2016

In https://github.com/JSON8/patch I return undefined

While that is compliant with the spec, it is unfortunate, as it breaks the expectation that the result remains a valid JSON document. I would recommend you change it to return null.

I believe the behavior we want should be explicit rather than implicit or unspecified. The spec defines the semantics of all that it defines. In the meantime, having divergence in implementations' behavior is unfortunate.

@sonnyp
Copy link

sonnyp commented Aug 25, 2016

JSON.stringify(undefined) returns "undefined" so the program won't break and it let the user decides what to do for the remove / case.

Anyway, this repo contains JSON Patch tests and since your proposed behavior is not specified I'd 👎

@espadrine
Copy link
Author

I'm not fond of the idea that removing the object converts it to the 9-character string "undefined" by default.

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