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
|
manandre
force-pushed
the
fileoptions-overloads
branch
from
fdc875c
to
fa5a930
Compare
stephentoub
added
the
NO-MERGE
|
Thanks for the PR. The APIs need to be revisited in light of #24698 (comment). |
manandre
force-pushed
the
fileoptions-overloads
branch
from
fa5a930
to
6dd9434
Compare
manandre
force-pushed
the
fileoptions-overloads
branch
from
6dd9434
to
763bfc0
Compare
manandre
force-pushed
the
fileoptions-overloads
branch
from
f305717
to
6d13d07
Compare
There was a problem hiding this comment.
@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>
adamsitnik
removed
the
NO-MERGE
There was a problem hiding this comment.
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.
Did we want to make this < rather than <= ? Or that'll be handled separately?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
I would expect to see FileStreamOptions being used here (as this is the ctor you are adding in this PR)
ghost
locked as resolved and limited conversation to collaborators
Closes #24698
/cc @Duranom @adamsitnik