Skip to content

Remove the trailing comma when removing the only property of an object - #147

Open
Alperen Kocakaplan (Pharex38) wants to merge 1 commit into
microsoft:mainfrom
Pharex38:remove-only-property-trailing-comma
Open

Alperen Kocakaplan (Pharex38) wants to merge 1 commit into
microsoft:mainfrom
Pharex38:remove-only-property-trailing-comma

Conversation

@Pharex38

Copy link
Copy Markdown

Removing the only property of an object that has a trailing comma leaves the comma behind:

applyEdits(text, modify('{"a": 1,}', ['a'], undefined, { formattingOptions }))
// '{\n  ,\n}'  -> CommaExpected/ValueExpected even with allowTrailingComma

The same happens for nested objects, e.g. removing the last setting from a section of a settings.json written with trailing commas. Arrays already handle it: the only-item case clears the brackets, so [1,] -> []. With two or more properties it works too, because then the edit spans to the next property, or starts at the previous one.

When the removed property is the only one, the edit now also covers a comma that directly follows it (comments and whitespace in between are skipped by the scanner). A comment after the comma is kept: { "x": "y", // note } -> { // note }.

Added remove the only property with a trailing comma to src/test/edit.test.ts. It fails without the change, and npm test passes (89 tests). I found it while fuzzing modify against parse; with the change, 20k random removals/inserts/sets all produce valid JSON with the expected value.

modify('{"a": 1,}', ['a'], undefined) produced '{ , }', which does not
parse even with allowTrailingComma. Arrays already handled this case.

@agammann Ammann Labs (agammann) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed the sole-property deletion path and compiled the TypeScript. All 89 tests passed. I also checked removal with and without formatting for nested objects, CRLF input, and comments before and after the comma. In each case, the property was removed and the resulting JSONC parsed without errors. This looks correct to me.

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