Add FileStreamOptions overloads by manandre · Pull Request #52720 · dotnet/runtime

manandre · 2021-05-13T19:16:06Z

ghost · 2021-05-13T19:16:11Z

Note regarding the new-api-needs-documentation label:

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.

ghost · 2021-05-13T19:16:17Z

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #24698

/cc @Duranom @adamsitnik

Author:	manandre
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`
Milestone:	-

stephentoub · 2021-05-25T01:21:24Z

Thanks for the PR. The APIs need to be revisited in light of #24698 (comment).

adamsitnik

@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.IO.FileSystem/tests/File/Open.cs

src/libraries/System.IO.FileSystem/tests/FileInfo/Open.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileInfo.cs

src/libraries/System.Runtime/ref/System.Runtime.cs

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

… enough

adamsitnik

LGTM, big thanks for your help @manandre, and once again apologies for the inconvenience from our side

stephentoub · 2021-06-08T10:45:47Z

src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs

        private static Stream ValidateArgsAndOpenPath(string path, Encoding encoding, int bufferSize)
+        {
+            ValidateArgs(path, encoding);
+            if (bufferSize <= 0)


Did we want to make this < rather than <= ? Or that'll be handled separately?

IMHO it should be handled separately in #53497

stephentoub · 2021-06-08T10:54:56Z

src/libraries/System.IO/tests/StreamWriter/StreamWriter.StringCtorTests.cs

+            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()));


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?

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?

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.

stephentoub · 2021-06-08T10:56:56Z

src/libraries/System.IO.FileSystem/tests/File/Open.cs

        }
    }

+    public class File_Open_str_fm_fa_fs_buffer_fo : FileStream_ctor_str_fm_fa_fs_buffer_fo


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.

I have aligned these tests with the existing options-based test classes from FileStream.

adamsitnik · 2021-06-09T06:40:01Z

src/libraries/System.IO/tests/StreamReader/StreamReader.cs

+            const string Text = "This is some text for testing.";
+            string path = GetTestFilePath();
+
+            using (var writer = new StreamWriter(path, false, encoding))


I would expect to see FileStreamOptions being used here (as this is the ctor you are adding in this PR)

ghost added area-System.IO new-api-needs-documentation labels May 13, 2021

manandre force-pushed the fileoptions-overloads branch from fdc875c to fa5a930 Compare May 15, 2021 11:46

manandre mentioned this pull request May 16, 2021

Add isAsync overload for Create/Open/etc. #24698

Closed

runfoapp bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 25, 2021

manandre force-pushed the fileoptions-overloads branch from fa5a930 to 6dd9434 Compare May 28, 2021 18:32

manandre added 5 commits May 28, 2021 20:37

Add FileOptions overloads to StreamReader and StreamWriter constructors

a91d814

Add FileOptions overloads to File and FileInfo

f878f5a

Fix missing namespace prefix in System.Runtime reference

1c4e12e

Fix net48 build

12f3d66

Move to option bag overloads

763bfc0

manandre force-pushed the fileoptions-overloads branch from 6dd9434 to 763bfc0 Compare May 28, 2021 18:59

Fix net48 build again

6d13d07

manandre force-pushed the fileoptions-overloads branch from f305717 to 6d13d07 Compare May 28, 2021 20:19

manandre changed the title ~~Add FileOptions overloads~~ Add FileStreamOptions overloads May 28, 2021

adamsitnik reviewed May 31, 2021

View reviewed changes

manandre and others added 2 commits May 31, 2021 22:02

Apply suggestions from code review

7cd83e9

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

Apply suggestions from code review continued

42bb7f6

manandre requested a review from adamsitnik June 1, 2021 16:09

adamsitnik added 2 commits June 8, 2021 10:54

reduce the number of new APIs, File.Open(FileStreamOptions) should be…

8f617d9

… enough

add missing docs

f8e0bcf

adamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 8, 2021

adamsitnik added this to the 6.0.0 milestone Jun 8, 2021

adamsitnik approved these changes Jun 8, 2021

View reviewed changes

adamsitnik mentioned this pull request Jun 8, 2021

DrawLines_PointFs, DrawBezier_PointFs, DrawBezier_Points and DrawLines_Points failing sporadically in CI dotnet/winforms#8814

Open

stephentoub reviewed Jun 8, 2021

View reviewed changes

stephentoub approved these changes Jun 8, 2021

View reviewed changes

stephentoub reviewed Jun 8, 2021

View reviewed changes

manandre added 2 commits June 8, 2021 22:31

Align on options-based FileStream tests

0711789

Add a test about encoding

ee37481

adamsitnik reviewed Jun 9, 2021

View reviewed changes

adamsitnik merged commit 8538436 into dotnet:main Jun 10, 2021

manandre deleted the fileoptions-overloads branch June 10, 2021 06:17

adamsitnik mentioned this pull request Jun 10, 2021

More file stream options tests #53982

Merged

ghost locked as resolved and limited conversation to collaborators Jul 10, 2021

Conversation

manandre commented May 13, 2021

Uh oh!

ghost commented May 13, 2021

Uh oh!

ghost commented May 13, 2021

Uh oh!

stephentoub commented May 25, 2021

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants