-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Mark Encoding as nullable in StreamWriter's constructor #106658
Conversation
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.
Can you please take into account #2376 (comment). I think we should align string
overloads with the Stream
ones as well as cover StreamReader
, and update the tests accordingly.
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.
Hi @stefannikolei !
Thank you for your contribution. Please update the reference file as well: https://github.com/dotnet/runtime/blob/9876ed1b72bb82018cdf18f0e8780c721cf806cd/src/libraries/System.Runtime/ref/System.Runtime.cs#L10752-L10755
I also changed the string based constructors and removed the checks for ArgumentNullException in the tests |
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.
Otherwise, LGTM.
src/libraries/System.Runtime/tests/System.IO.Tests/StreamWriter/StreamWriter.StringCtorTests.cs
Show resolved
Hide resolved
83a01fe
to
4591bff
Compare
/ba-g #104340 (comment). |
* Mark Encoding as nullable in StreamWriter's constructor * Update reference file * Align string constructors with Stream * add explicit test for null as encoding
* Mark Encoding as nullable in StreamWriter's constructor * Update reference file * Align string constructors with Stream * add explicit test for null as encoding
Fixes #106237