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

Make string content and heredoc content visible #127

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Nov 2, 2020

The contents of string literals and heredocs used to be hidden because the rule names for them start with an _.

@dcreager

src/grammar.json Outdated
@@ -5035,7 +5035,7 @@
},
{
"type": "PATTERN",
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*/\\\\%?!~()\\[\\]{}]*"
"value": "[^\\x00-\\x1F\\s:;`\"'@$#.,|^&<=>+\\-*\\/\\\\%?!~()\\[\\]{}]*"
Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:;`\"'@$#.,|^&<=>+\\-*\\/\\\\%?!~()\\[\\]{}]*"
Copy link
Contributor

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.

@dcreager dcreager merged commit 11ac2b3 into tree-sitter:master Nov 3, 2020
maxbrunsfeld added a commit that referenced this pull request Nov 3, 2020
Instead, just make the tokens themselves visible.
Refs #127

/cc @aibaars - This is just a slight simplification on your PR.
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.

None yet

2 participants