-
Notifications
You must be signed in to change notification settings - Fork 56
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
Make string content and heredoc content visible #127
Conversation
1647e0e
to
c66fa1c
Compare
src/grammar.json
Outdated
@@ -5035,7 +5035,7 @@ | |||
}, | |||
{ | |||
"type": "PATTERN", | |||
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*/\\\\%?!~()\\[\\]{}]*" | |||
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*\\/\\\\%?!~()\\[\\]{}]*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxbrunsfeld : I wonder where these changes come from. They seem to revert the changes of 7608c14 . I ran node node_modules/tree-sitter-cli/cli.js generate
to re-generate the files. Did you use another version tree-sitter
when you generated the files?
node node_modules/tree-sitter-cli/cli.js -V
tree-sitter 0.17.3 (c439a676cf169e88234f768ca0f69d42e5bd68c5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks innocuous, since it's just adding an escape to a forward slash. I'm also curious where it came from. I briefly tried bisecting this but couldn't find anything obvious. The JavaScript standard says that slashes should be escaped (and if I'm reading it carefully, it furthermore says that slashes should always be escaped). But the current V8 source looks like it only escapes slashes that occur outside of a character class. This example is inside a character class, so now I'm thoroughly confused.
Anyway, since it shouldn't affect the result, I'm 👍 to merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A-ha! Someone else has done the bisecting: nodejs/node#29888, v8/v8@17b9d87
Slashes inside of character classes are no longer escaped as of V8 7.7.206 and node.js 12.11.0. @aibaars can you double-check what version of node.js you have installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had version 10 installed. I'll upgrade and regenerate the files.
src/grammar.json
Outdated
@@ -5035,7 +5035,7 @@ | |||
}, | |||
{ | |||
"type": "PATTERN", | |||
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*/\\\\%?!~()\\[\\]{}]*" | |||
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*\\/\\\\%?!~()\\[\\]{}]*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks innocuous, since it's just adding an escape to a forward slash. I'm also curious where it came from. I briefly tried bisecting this but couldn't find anything obvious. The JavaScript standard says that slashes should be escaped (and if I'm reading it carefully, it furthermore says that slashes should always be escaped). But the current V8 source looks like it only escapes slashes that occur outside of a character class. This example is inside a character class, so now I'm thoroughly confused.
Anyway, since it shouldn't affect the result, I'm 👍 to merge this.
fe91a88
to
0f2fe0b
Compare
The contents of string literals and heredocs used to be hidden because the rule names for them start with an
_
.@dcreager