-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Changes from 10 commits
a91d814
f878f5a
1c4e12e
12f3d66
763bfc0
6d13d07
7cd83e9
42bb7f6
8f617d9
f8e0bcf
0711789
ee37481
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,15 @@ public class StreamWriter_StringCtorTests | |
public static void NullArgs_ThrowsArgumentNullException() | ||
{ | ||
AssertExtensions.Throws<ArgumentNullException>("path", () => new StreamWriter((string)null)); | ||
AssertExtensions.Throws<ArgumentNullException>("path", () => new StreamWriter((string)null, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("path", () => new StreamWriter((string)null, true)); | ||
AssertExtensions.Throws<ArgumentNullException>("path", () => new StreamWriter((string)null, true, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("path", () => new StreamWriter((string)null, true, null, -1)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("path", true, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("path", null, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("path", true, null, -1)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("", true, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("", null, null)); | ||
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamWriter("", true, null, -1)); | ||
} | ||
|
||
|
@@ -29,8 +32,10 @@ public static void EmptyPath_ThrowsArgumentException() | |
{ | ||
// No argument name for the empty path exception | ||
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("")); | ||
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("", new FileStreamOptions())); | ||
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 commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
AssertExtensions.Throws<ArgumentException>(null, () => new StreamWriter("", true, Encoding.UTF8, -1)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.IO | ||
{ | ||
public static partial class File | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="System.IO.FileStream" /> class with the specified path, creation mode, read/write and sharing permission, the access other FileStreams can have to the same file, the buffer size, additional file options and the allocation size. | ||
/// </summary> | ||
/// <remarks><see cref="System.IO.FileStream(string,System.IO.FileStreamOptions)"/> for information about exceptions.</remarks> | ||
public static FileStream Open(string path, FileStreamOptions options) => new FileStream(path, options); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
namespace System.IO | ||
{ | ||
public sealed partial class FileInfo : FileSystemInfo | ||
{ | ||
/// <summary> | ||
/// Initializes a new instance of the <see cref="System.IO.FileStream" /> class with the specified creation mode, read/write and sharing permission, the access other FileStreams can have to the same file, the buffer size, additional file options and the allocation size. | ||
/// </summary> | ||
/// <remarks><see cref="System.IO.FileStream(string,System.IO.FileStreamOptions)"/> for information about exceptions.</remarks> | ||
public FileStream Open(FileStreamOptions options) => File.Open(NormalizedPath, options); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,23 +193,47 @@ public StreamReader(string path, Encoding encoding, bool detectEncodingFromByteO | |
{ | ||
} | ||
|
||
public StreamReader(string path, Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize) : | ||
this(ValidateArgsAndOpenPath(path, encoding, bufferSize), encoding, detectEncodingFromByteOrderMarks, bufferSize, leaveOpen: false) | ||
public StreamReader(string path, Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize) | ||
: this(ValidateArgsAndOpenPath(path, encoding, bufferSize), encoding, detectEncodingFromByteOrderMarks, bufferSize, leaveOpen: false) | ||
{ | ||
} | ||
|
||
public StreamReader(string path, FileStreamOptions options) | ||
: this(path, Encoding.UTF8, true, options) | ||
{ | ||
} | ||
|
||
public StreamReader(string path, Encoding encoding, bool detectEncodingFromByteOrderMarks, FileStreamOptions options) | ||
: this(ValidateArgsAndOpenPath(path, encoding, options), encoding, detectEncodingFromByteOrderMarks, DefaultBufferSize) | ||
{ | ||
} | ||
|
||
private static Stream ValidateArgsAndOpenPath(string path, Encoding encoding, FileStreamOptions options) | ||
{ | ||
ValidateArgs(path, encoding); | ||
if (options == null) | ||
throw new ArgumentNullException(nameof(options)); | ||
|
||
return new FileStream(path, options); | ||
} | ||
|
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. IMHO it should be handled separately in #53497 |
||
throw new ArgumentOutOfRangeException(nameof(bufferSize), SR.ArgumentOutOfRange_NeedPosNum); | ||
|
||
return new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultFileStreamBufferSize); | ||
} | ||
|
||
private static void ValidateArgs(string path, Encoding encoding) | ||
{ | ||
if (path == null) | ||
throw new ArgumentNullException(nameof(path)); | ||
if (encoding == null) | ||
throw new ArgumentNullException(nameof(encoding)); | ||
if (path.Length == 0) | ||
throw new ArgumentException(SR.Argument_EmptyPath); | ||
if (bufferSize <= 0) | ||
throw new ArgumentOutOfRangeException(nameof(bufferSize), SR.ArgumentOutOfRange_NeedPosNum); | ||
|
||
return new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultFileStreamBufferSize, FileOptions.SequentialScan); | ||
} | ||
|
||
public override void Close() | ||
|
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
.