-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix missing comma classification in json classification #68535
Conversation
@@ -187,7 +197,7 @@ public void Visit(JsonTextNode node) | |||
|
|||
public void Visit(JsonCommaValueNode node) | |||
{ | |||
AddClassification(node.CommaToken, ClassificationTypeNames.JsonPunctuation); | |||
// already handled when we recurse in AddTokenClassifications |
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 don't think I understand how the logic in here works. Why are commas special versus other types of tokens handled by the other visit methods?
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.
Commas are handled slightly specially in json (for error recovery purposes). They can both be a node (with a token in them), or just a token in separated list. We were only handling the first case.
Now we handle both cases by just checking the token case only.
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.
Probably worth documenting that :)
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.
That's what I thought that comment was :'(
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 comment explains a bit of the why, but not the "commas are special for error recovery" part. That's what I needed to know to understand the code 🙂.
Fixes #68534