-
Notifications
You must be signed in to change notification settings - Fork 392
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
Parser server error handling improvements #3035
Parser server error handling improvements #3035
Conversation
fef6d15
to
35ecfba
Compare
.Which | ||
.ErrorMessage | ||
.Message | ||
.Should() | ||
.Contain($"Unable to parse an interactive document with type '{(int)request.SerializationType}'"); |
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 is orthogonal - but should our exception messages be localized?
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.
We don't typically localize exception messages.
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.
My assumption was that exception messages should be localized if they can ever end up being displayed to the end user...
For C#, we would display localized messages for compilation errors etc., would we not? It seems strange that we would always display English messages for any errors that originate within .NET Interactive itself...
@@ -33,7 +33,7 @@ internal static class StringExtensions | |||
} | |||
else if (lines.Length > 1) | |||
{ | |||
firstLine = firstLine + " ..."; | |||
// firstLine = firstLine + " ..."; |
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.
Was this intentional or something temporary that you meant to revert?
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.
Ah, I didn't mean to revert it. The logic looked weird so I was checking if it's actually needed.
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 is just appending ellipsis if the content happens to span multiple lines (since we are trimming out all lines except the first one). I think it should be preserved.
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'll replace this in my next PR.
This PR:
dotnet-interactive
.NotebookErrorResponse
type and sends error info to log output rather than trying to communicate them over the main channel.