-
Notifications
You must be signed in to change notification settings - Fork 381
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
Update kernel to support dynamic variables #3424
Conversation
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
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.
The changes overall look reasonable. However,
-
I suspect there are some vestiges of the old logic still lingering in the code that was ported over from the old parser. And seems like this could result in incorrect / missing errors (e.g. in the
{{$guid $guid}}
case). The comments around these functional aspects seem like the most important ones to address first. -
We are also missing tests for some of the new error messages and for some negative cases such as the
{{$guid $guid}}
case above. It would be great to include some tests for these missing cases. -
Other than that, most of the comments are around code hygiene, some improvements for the error messages we report and some improvement for tests. I am ok with fixing these up in follow up PRs (so long as we fix up the error messages during the preview releases of the new parser integration).
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Show resolved
Hide resolved
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.
The changes look good, and the test coverage is much improved now.
However, I found a couple of bugs and few more cases that need tests. I am fine with merging the PR so long as the corresponding comments are addressed. Please take a look.
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpDiagnostics.cs
Outdated
Show resolved
Hide resolved
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.
Looks like you may have missed some of the comments because GitHub collapses them by default in the discussion view. Let's sync up tomorrow.
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.
👍🏾 Looks great. Thanks for adding test coverage for all the cases!
Added support to the parser to compute several types of additional variables in request