Skip to content

Improve consistency of StreamReader and StreamWriter #118958

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Sep 19, 2025
Merged

Improve consistency of StreamReader and StreamWriter #118958

merged 11 commits into from
Sep 19, 2025

Conversation

CloudeaSoft
Copy link
Contributor

@CloudeaSoft CloudeaSoft commented Aug 21, 2025 •
edited
Loading

Fix #118748

  1. Remove const DefaultFileStreamBufferSize.
  2. Set DefaultBufferSize (1024) as the default param of the constructor overload that takes FileStreamOptions in StreamWriter.
  3. Allow user to pass -1 to use the default buffer size in constructors that take a file path, instead of encountering an ArgumentOutOfRangeException.
  4. Update tests to adapt to the change 3.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 21, 2025
@jkotas jkotas changed the title Enhance cconsistency of StreamReader and StreamWriter Enhance consistency of StreamReader and StreamWriter Aug 21, 2025
@build-analysis build-analysis bot mentioned this pull request Aug 22, 2025
Open
@build-analysis build-analysis bot mentioned this pull request Aug 22, 2025
Closed
@build-analysis build-analysis bot mentioned this pull request Aug 23, 2025
Open
3 tasks
This was referenced Aug 24, 2025
Open
Closed
Open
@CloudeaSoft CloudeaSoft changed the title Enhance consistency of StreamReader and StreamWriter Improve consistency of StreamReader and StreamWriter Aug 25, 2025
jeffhandley
jeffhandley approved these changes Sep 2, 2025
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. @jozkee; I'd like you and @ViveliDuCh to review this together and make sure nothing else needs to be changed at the same time.

After this is merged, please create a PR to update the API docs to add a remark about this behavior too.

jozkee
jozkee approved these changes Sep 17, 2025
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments to consider, thanks.

@CloudeaSoft
Copy link
Contributor Author

@ViveliDuCh . Sry, what's the meaning of commit e937c9d ? Is this means the improvements are no longer suggested, or they should be commited by myself instead of a reviewer?

@ViveliDuCh
Copy link
Member

@ViveliDuCh . Sry, what's the meaning of commit e937c9d ? Is this means the improvements are no longer suggested, or they should be commited by myself instead of a reviewer?

Thanks for asking!
The commit e937c9d was made to fix a misunderstanding when addressing @jozkee’s earlier suggestions. Specifically, the initial change updated the defaultValue in the constructors, but David’s suggestion was to update it only in ValidateArgsAndOpenPath. This commit corrects that by keeping the change just for the correct locations. It also includes an update to a comment for improved consistency.
To clarify, only these particular suggestions (the comment update and changing the defaultValue in ValidateArgsAndOpenPath) have been addressed in this commit. Let me know if you have any further questions!

@jozkee jozkee enabled auto-merge (squash) September 19, 2025 02:34
@jozkee jozkee merged commit 2f1efc2 into dotnet:main Sep 19, 2025
142 of 144 checks passed
@build-analysis build-analysis bot mentioned this pull request Sep 19, 2025
Closed
jkotas
jkotas reviewed Sep 19, 2025
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/IO/StreamReader.cs
// Creates a new StreamReader for the given stream. The
// character encoding is set by encoding and the buffer size,
// in number of 16-bit characters, is set by bufferSize.
// in bytes, is set by bufferSize.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also submit a PR to https://github.com/dotnet/dotnet-api-docs/ to make this change in the official API docs?

(The file to edit is https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.IO/StreamReader.xml)

This was referenced Sep 19, 2025
Closed
Merged
xtqqczze pushed a commit to xtqqczze/dotnet-runtime that referenced this pull request Sep 20, 2025
@CloudeaSoft @ViveliDuCh @xtqqczze
Improve consistency of StreamReader and StreamWriter (dotnet#118958)
8caf7e2 
* Enhance consistency of StreamReader and StreamWriter

* Fix path missing error in StringCtorTests of StreamReader and StreamWriter

* Fix a param error in StreamWriter.StringCtorTests.cs

* PR change suggestions

* Revert change for constructors

* Fix whitespace

* Add buffer size test in stream ctor tests

---------

Co-authored-by: Viviana Dueñas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkotas jkotas jkotas left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@jozkee jozkee jozkee approved these changes

@ViveliDuCh ViveliDuCh ViveliDuCh approved these changes

+1 more reviewer

@lilinus lilinus lilinus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jozkee jozkee

Labels

area-System.IO community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

StreamWriter/StreamReader buffer size inconsistencies

6 participants

@CloudeaSoft @ViveliDuCh @jeffhandley @jkotas @jozkee @lilinus