Skip to content
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 InternalProcessStreamReader.cs #11

Merged
merged 1 commit into from
May 31, 2017
Merged

Conversation

xjfnet
Copy link
Contributor

@xjfnet xjfnet commented May 29, 2017

InternalProcessStreamReader encoding must be same as original StreamReader.
Process StandardOutput is Encoding.Default, but StreamReader is Encoding.Utf8 by default, it will leads to messy code when Command.StandardOutput.ReadLine() or Command.StandardOutput.ReadToEnd().

InternalProcessStreamReader encoding must be same as original StreamReader
@madelson
Copy link
Owner

Hi @xjfnet thanks for contributing!

This looks like a very reasonable change. It looks like ProcessStartInfo already allows for the stdin/stdout encodings to be specified, so with this people can now leverage the API for customizing start info in order to work around any encoding problems, which is great. A few thoughts/questions:

  • I'd like for the codebase to have a unit test that fails under the UTF8 case to explicitly demonstrate the problem and the correctness of the new code.
  • We should also have a test case that demonstrates sending international characters across the wire to show that switching away from UTF8 doesn't break things.

For these test cases, I'm happy to have you add them yourself as part of this PR, or I can add them after merging. Let me know what you want to do!

@madelson madelson merged commit 14bb45c into madelson:master May 31, 2017
@madelson
Copy link
Owner

madelson commented May 31, 2017

This fix is part of 1.3.0 (https://www.nuget.org/packages/MedallionShell/1.3.0). Thanks again for submitting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants