-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add FileStreamOptions overloads #52720
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @carlossanlop Issue DetailsCloses #24698 /cc @Duranom @adamsitnik
|
fdc875c
to
fa5a930
Compare
Thanks for the PR. The APIs need to be revisited in light of #24698 (comment). |
fa5a930
to
6dd9434
Compare
6dd9434
to
763bfc0
Compare
f305717
to
6d13d07
Compare
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.
@manandre big thanks for your contribution and apologies for the re-design of the APIs and merge conflicts. I've left some comments and suggestions, PTAL.
src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
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.
LGTM, big thanks for your help @manandre, and once again apologies for the inconvenience from our side
private static Stream ValidateArgsAndOpenPath(string path, Encoding encoding, int bufferSize) | ||
{ | ||
ValidateArgs(path, encoding); | ||
if (bufferSize <= 0) |
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.
Did we want to make this < rather than <= ? Or that'll be handled separately?
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.
IMHO it should be handled separately in #53497
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("", true)); | ||
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("", true, Encoding.UTF8)); | ||
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("", Encoding.UTF8, new FileStreamOptions())); |
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 only see negative tests here for StreamReader/Writer. Are there tests that validate the ctors do what they're supposed to do when used correctly?
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 fully agree they are missing, but I did not find any 'positive' test covering the existing constructors with path, FileMode, FileAccess, FileShare, bufferSize and FileOptions parameters.
Which type of tests do you expect here?
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.
Which type of tests do you expect here?
I was primarily thinking about tests to validate that we're correctly passing through the arguments, e.g. that if you pass CustomEncoding as the encoding, that's actually what gets used by the writer.
@@ -42,6 +42,24 @@ protected override FileStream CreateFileStream(string path, FileMode mode, FileA | |||
} | |||
} | |||
|
|||
public class File_Open_str_fm_fa_fs_buffer_fo : FileStream_ctor_str_fm_fa_fs_buffer_fo |
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.
Is this test name correct? I thought the pattern was based on the arguments being accepted by the method, but these methods are all path,options rather than path,FileMode,FileAccess,FileShare,bufferSize,FileOptions.
Same for the subsequent change.
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 have aligned these tests with the existing options-based test classes from FileStream
.
const string Text = "This is some text for testing."; | ||
string path = GetTestFilePath(); | ||
|
||
using (var writer = new StreamWriter(path, false, encoding)) |
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 would expect to see FileStreamOptions
being used here (as this is the ctor you are adding in this PR)
Closes #24698
/cc @Duranom @adamsitnik