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

Clarify whether / how Edit[] can be concatenated #53

Closed
Marcono1234 opened this issue Aug 31, 2021 · 3 comments
Closed

Clarify whether / how Edit[] can be concatenated #53

Marcono1234 opened this issue Aug 31, 2021 · 3 comments

Comments

@Marcono1234
Copy link
Contributor

The documentation currently does not make it clear whether or how Edit[] from multiple modify() or format() calls can be concatenated before being applied using applyEdits().

Both the documentation for modify() and format() contains the following:

Edits can be either inserts, replacements or removals of text segments. All offsets refer to the original state of the document. No two edits must change or remove the same range of text in the original document. However, multiple edits can have the same offset, for example multiple inserts, or an insert followed by a remove or replace. The order in the array defines which edit is applied first.

Personally I think this documentation should be moved to applyEdits() respectively to the documentation of the type Edit instead since it is more relevant there.

The issue is that the output of multiple modify() calls cannot be concatenated without risking to produce malformed JSON when calling applyEdits(). Here are two examples (from #48 (comment)):

  • Removing subsequent array elements / object properties.
    This will cause both elements / properties to remove the same ,, which in the end results in one character too much being removed:
    const json = '{"a":1,"b":2}'
    let edits = jsoncParser.modify(json, ["a"], undefined, {})
    edits = edits.concat(jsoncParser.modify(json, ["b"], undefined, {}))
    
    // Prints only `{`; the closing `}` is missing
    console.log(jsoncParser.applyEdits(json, edits))
  • Removing array elements / object properties in reverse order:
    const json = '{"a":1,"b":2,"c":3}'
    let edits = jsoncParser.modify(json, ["c"], undefined, {})
    edits = edits.concat(jsoncParser.modify(json, ["a"], undefined, {}))
    
    // Prints `{"b":2,"c":3`
    console.log(jsoncParser.applyEdits(json, edits))

So currently the only safe usage of modify() and applyEdits() is to call both functions for every modification you want to make. It is not safe to try concatenating the result of multiple modify() calls. This is rather inefficient when a user wants to remove multiple properties.

In case this is intended, then it should ideally be clarified in the documentation for the modify() (and format()?) function.

aeschli added a commit that referenced this issue Oct 25, 2021
@aeschli
Copy link
Contributor

aeschli commented Oct 25, 2021

The results of multiple modify (and or format) operations can't be concatenated unless you are sure they don't impact each other.

A different API is needed that would let you compute the text operations for more than one JSON modification. I don't have plans to implement such functionality.

I tried to improve the spec around edit operations.

@aeschli aeschli closed this as completed Oct 25, 2021
@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Oct 26, 2021

Thanks! Would it be possible to add one more sentence to the EditResult documentation, mentioning what you wrote in your comment? That is (for example):

In general multiple EditResults must not be concatenated because they might impact each other, producing incorrect or malformed JSON documents.

I think even with your improvements that is currently still not clear.

aeschli added a commit that referenced this issue Oct 26, 2021
@aeschli
Copy link
Contributor

aeschli commented Oct 26, 2021

Thanks for the suggestion, I added it!

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

No branches or pull requests

2 participants