-
Notifications
You must be signed in to change notification settings - Fork 419
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
config encoding For Stdio #598
Conversation
Awesome -- this was on my list to go look at. |
This is a good change! Though it looks like |
FWIW, getting the Default encoding is kind of a nightmare. Here's what we do in the Roslyn compiler: http://source.roslyn.io/#Microsoft.CodeAnalysis/EncodedStringText.cs,ff93fabe53148f02. |
I talked with "reluctant encoding guru" @jaredpar, and his advice was to not use Default at all. It's entirely possible for two processes to have different values for Default and these should be the same for client and server. IMO, we should just pick one to be our enforce it. I vote for UTF8. |
The other option is to just not set anything if the --encoding flag is not set. |
@@ -30,6 +30,7 @@ public static void Main(string[] args) | |||
var otherArgs = new List<string>(); | |||
var plugins = new List<string>(); | |||
var serverInterface = "localhost"; | |||
var encoding = System.Text.Encoding.Default; |
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.
Add using for System.Text.
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.
Since Encoding.Default is not available on .NET Core, it's probably best to just not set the encoding if the --encoding
is not specified. So, set encoding
to null here and then check for null later. E.g.
Encoding encoding = null;
@nabychan: I'm very happy to merge this once you make the suggested changes and the CI build passes. Thanks a ton for your help! |
I think "reluctant encoding guru" roughly translates to "been burned by this before" 😄 |
@DustinCampbell OK, I'll have try to make it better |
Update MAINTAINERS
👍 |
Thanks again for your work on this! |
@@ -100,6 +107,11 @@ public static void Main(string[] args) | |||
|
|||
if (transportType == TransportType.Stdio) | |||
{ | |||
if(encoding != null) |
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.
nit: space between if
and (
Looks great @nabychan |
Thanks @DustinCampbell . My first time to participate in projects from GitGub . Not that fluently . |
vs code need utf-8 encoding for stdio,and other maybe not.