Ensure that comments are stripped before reading tasks.json file #1075
Conversation
… calling JSON.parse(...)
| @@ -357,7 +358,7 @@ function getBuildOperations(tasksJsonPath: string) { | |||
| } | |||
|
|
|||
| const text = buffer.toString(); | |||
| const tasksJson: tasks.TaskConfiguration = JSON.parse(text); | |||
| const tasksJson: tasks.TaskConfiguration = tolerantParse(text); | |||
There was a problem hiding this comment.
Should this have a try/catch around it?
There was a problem hiding this comment.
It should, but it didn't before. I'll add one.
| } | ||
|
|
||
| export function tolerantParse(text: string) { | ||
| text = stripComments(text); |
There was a problem hiding this comment.
Should we ignore the UTF8-bom while we are at it too?
There was a problem hiding this comment.
Yes, we should. Also, I'd like to add tolerance around trailing commas as well.
There was a problem hiding this comment.
OK. Added code to be tolerant against both byte order marks and trailing comments in object member or array element lists.
| let nextCode = peekPastWhitespace(); | ||
| if (nextCode === CharCode.closeBrace || nextCode === CharCode.closeBracket) { | ||
| parts.push(text.substring(partStart, index - 1)); | ||
| partStart = index; |
There was a problem hiding this comment.
don't you need to know the index of the comma in the case that peekPastWhitespace did something?
There was a problem hiding this comment.
peekPastWhitespace doesn't do anything but look ahead. It doesn't modify index
There was a problem hiding this comment.
Oh, never mind. I was thinking that partStart was going to get the wrong value, but this works.
| let result = JSON.stringify(json, null, 4); | ||
|
|
||
| result.should.equal(expected); | ||
| }); |
There was a problem hiding this comment.
Add a test for something where the extra , is preceeded by whitespace? (ex:
{
"obj" : { "a": 1 , }
}There was a problem hiding this comment.
Just thought of an issue. This won't work when a trailing comma has a comment at the end of the line. Fixing...
There was a problem hiding this comment.
Actually, I'm going to let that go for now.
There was a problem hiding this comment.
It'll require significant refactoring
There was a problem hiding this comment.
Filed #1076 to track this.
|
LGTM |
Fixes #1074