Add in a json parser so that the IDE can provide services around json editing. #24110
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! |
CyrusNajmabadi
force-pushed
the
jsonParsing
branch
from
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?
|
|
||
| internal static class JsonHelpers | ||
| { | ||
| public static JsonToken CreateToken(JsonKind kind, ImmutableArray<JsonTrivia> leadingTrivia, ImmutableArray<VirtualChar> virtualChars, ImmutableArray<JsonTrivia> trailingTrivia) |
|
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); |
|
|
||
| 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.
| 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; |
| 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.
| 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
CyrusNajmabadi
force-pushed
the
jsonParsing
branch
from
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: