Add in a json parser so that the IDE can provide services around json editing.#24110
Add in a json parser so that the IDE can provide services around json editing.#24110CyrusNajmabadi wants to merge 37 commits intodotnet:features/embeddedJsonfrom
Conversation
|
Tagging @dotnet/roslyn-ide @Pilchie |
|
@CyrusNajmabadi Should we wait until the regex one is resolved prior to looking at this one? This PR is quite, quite large. |
That woudl be my recommendation :)
It's a mere 46k lines. If you do 2k lines an hour, you can finish it by tomorrow! |
|
Options look like this:
|
|
Can we just keep you in various airports? 😆 |
|
Tomorrow LaGuardia! |
c06a692 to
682762e
Compare
| ImmutableArray<JsonTrivia> trailingTrivia, ImmutableArray<EmbeddedDiagnostic> diagnostics) | ||
| => CreateToken(kind, leadingTrivia, virtualChars, trailingTrivia, diagnostics, value: null); | ||
|
|
||
| public static JsonToken CreateToken(JsonKind kind, |
There was a problem hiding this comment.
This method is only used once (on line 20). Inline?
There was a problem hiding this comment.
done. #resolved.
|
|
||
| internal static class JsonHelpers | ||
| { | ||
| public static JsonToken CreateToken(JsonKind kind, ImmutableArray<JsonTrivia> leadingTrivia, ImmutableArray<VirtualChar> virtualChars, ImmutableArray<JsonTrivia> trailingTrivia) |
There was a problem hiding this comment.
done. #resolved.
|
Could you retarget to the feature branch? |
| { | ||
| internal enum JsonKind | ||
| { | ||
| None, |
There was a problem hiding this comment.
Is this used? If not, maybe None = 0 or remove?
There was a problem hiding this comment.
i like this as it makes it easy to test if a token is default.
There was a problem hiding this comment.
Sure. i can do that.
| Text = text; | ||
| } | ||
|
|
||
| public VirtualChar CurrentChar => Position < Text.Length ? Text[Position] : new VirtualChar((char)0, default); |
There was a problem hiding this comment.
done. #resolved.
|
|
||
| public VirtualChar CurrentChar => Position < Text.Length ? Text[Position] : new VirtualChar((char)0, default); | ||
|
|
||
| public ImmutableArray<VirtualChar> GetSubPattern(int start, int end) |
There was a problem hiding this comment.
end is generally Position. Consider adding an overload where end is implicit.
There was a problem hiding this comment.
done. #resolved.
| case '\'': case '"': | ||
| return ScanString(); | ||
|
|
||
| //case '-': case '.': |
There was a problem hiding this comment.
Added comment explaining why we do not explicitly lex out numbers here, but instead let the parser take control. #resolved.
| private (ImmutableArray<VirtualChar>, JsonKind, EmbeddedDiagnostic?) ScanString() | ||
| { | ||
| var start = Position; | ||
| var startChar = this.CurrentChar.Char; |
There was a problem hiding this comment.
done. #resolved.
| return (chars, JsonKind.StringToken, diagnostic); | ||
| } | ||
|
|
||
| private EmbeddedDiagnostic? ScanEscape(int stringStart, int escapeStart) |
There was a problem hiding this comment.
ScanEscape could use a comment since unlike other scan methods, it doesn't return tokens or nodes, it just advances the position.
There was a problem hiding this comment.
done. #Resolved
| return (GetSubPattern(start, Position), JsonKind.TextToken, null); | ||
| } | ||
|
|
||
| private (ImmutableArray<VirtualChar>, JsonKind, EmbeddedDiagnostic?)? TryScanText( |
There was a problem hiding this comment.
indeed. removed. #resolved.
| var chars = GetSubPattern(start, Position); | ||
| if (Position == start + 2) | ||
| { | ||
| var diagnostics = ImmutableArray.Create(new EmbeddedDiagnostic( |
There was a problem hiding this comment.
What if // are the last two characters in the file?
There was a problem hiding this comment.
then you get an unterminated comment diagnostic. This matches json.net. You can see a test for that here: https://github.com/dotnet/roslyn/pull/25521/files#diff-d0475d1c64e0a4bb26665ae88ff3cf24R158
Will add a comment explaining this is intentional.
There was a problem hiding this comment.
added comment. #resolved.
| GetTextSpan(start, Position)))); | ||
| } | ||
|
|
||
| public TextSpan GetTextSpan(int startInclusive, int endExclusive) |
There was a problem hiding this comment.
This method seems to only be used once. private?
There was a problem hiding this comment.
made privat.e #resolved
|
FYI: JetBrains have implemented support for the short form "lang=" (as well as the original "language=" form). |
Were you asking me to support both those forms? If so, i already do :) Also, would you be able to assist in any way with teh reviews on this. It looks like this has completely stalled out and isn't making any forward progress, even after 6 months... |
|
When reviewing it, I noticed that you implemented lang= and language=. As for progress, it's just a matter of resourcing. The team has been prioritizing other features for 15.8. (You know how it is.) |
840fa0c to
61f91ca
Compare
|
@sharwell @jinujoseph How would you like to proceed on this? |
1 similar comment
|
@sharwell @jinujoseph How would you like to proceed on this? |
|
Unfortunately, This feature is currently not in the top priority for us to pursue, so this is on hold at this moment. Following up to see if we can get merge this to feature branch |
|
@CyrusNajmabadi as discussed closing this PR for now. |
Followup to #23984
So JFK has busted pipes. So i've been in airport limbo :-( On a good note, it gave me some time to expand on my previous Regex PR. Now, on top of Regex support in the IDE we also support JSON strings.
This also expands the previous interaction model with built in json detection for string tokens. i.e., if we see:
Then we'll offer:
This then helps users see how they can add a comment that lights-up IDE features for these types of strings. Note: i don't do this for regexes because of a concern about too many false positives. Once the comment is added things like up like so:
Of course, squiggles and braces are supported, on top of just colors:
Also, if you're using JToken/JObject/JArray, then you don't need to provide the comment (similar to how
new Regex(pattern)is automatically detected:Things to note: