Skip to content
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

Mark or set encoding param in StreamReader ctor to be nullable #108542

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Mark or set encoding param in StreamReader ctor to be nullable #108542

wants to merge 4 commits into from

Conversation

eriawan
Copy link
Member

@eriawan eriawan commented Oct 4, 2024

Fixes #106236

@dotnet-issue-labeler dotnet-issue-labeler
Copy link

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, 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.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

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, 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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 4, 2024
@eriawan
Copy link
Member Author

eriawan commented Oct 4, 2024

@jozkee please review

cc @rameel

@build-analysis build-analysis bot mentioned this pull request Oct 4, 2024
Open
3 tasks
@lilinus
Copy link
Contributor

lilinus commented Oct 4, 2024

Should the string path constructors also be modified to allow null (similar to #106658, as discussed in #2376 (comment))? I.e. the ctors below:

public StreamReader(string path, System.Text.Encoding encoding)
public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks)
public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize)
public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, System.IO.FileStreamOptions options)

Also; are there any tests that could be modified to verify null encoding works (and acts as UTF8)?

@rameel
Copy link

rameel commented Oct 4, 2024

Yes, as @lilinus mentioned, the constructors that accept path and encoding parameters also need to be aligned with the Stream-based constructors, similar to how it was done for StreamWriter.

@eriawan
Copy link
Member Author

eriawan commented Oct 5, 2024

Yes, as @lilinus mentioned, the constructors that accept path and encoding parameters also need to be aligned with the Stream-based constructors, similar to how it was done for StreamWriter.

thanks for the feedback! sure, I'm gong to update now.

@eriawan
Copy link
Member Author

eriawan commented Oct 5, 2024

@lilinus @rameel

Done updating the ccror with the string path as well.

@jozkee
Could you please review?

@eriawan
Copy link
Member Author

eriawan commented Oct 5, 2024

Ok, I also have done refactoring the test of StreamReader's constructor as well.

@eriawan eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from 1d796f1 to c74bd44 Compare October 9, 2024 17:15
@eriawan
Copy link
Member Author

eriawan commented Oct 10, 2024

@dotnet/area-system-io team (or area owner),

Could you review this PR? Hopefully this PR can be reviewed sooner, as it's been more than 5 days...

cc @jozkee @rameel

@eriawan
Copy link
Member Author

eriawan commented Oct 11, 2024

@dotnet/area-system-io friends, could you take a look/review here?

Also maybe you could suggest or explain why the pipeline of "runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)" is running for more than 12 hours and never ends? Or should I just close this PR?

cc @jozkee @adamsitnik

@lilinus
Copy link
Contributor

lilinus commented Oct 11, 2024

From my personal experience, it can take some time before a PR gets reviewed/merged. I imagine the team also has a lot of critical work to complete for .NET 9 they need to prioritize.

I would have patience, it is gonna get reviewed eventually 😄

@eriawan
Copy link
Member Author

eriawan commented Oct 11, 2024

From my personal experience, it can take some time before a PR gets reviewed/merged. I imagine the team also has a lot of critical work to complete for .NET 9 they need to prioritize.

I would have patience, it is gonna get reviewed eventually 😄

got it, thanks!
I usually try to remind them as well, as I have done before.

@eriawan
Copy link
Member Author

eriawan commented Oct 22, 2024

hi @dotnet/area-system-io team

Just to remind, could you please review this PR? It's been about 2 weeks now...
Thanks in advance.

cc @jozkee @adamsitnik

@eriawan eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from c74bd44 to b7a69c9 Compare October 22, 2024 20:29
@eriawan
Copy link
Member Author

eriawan commented Oct 22, 2024

Rebase this to main branch to ensure my PR is up to date as of 23rd October.

This was referenced Oct 23, 2024
Open
Closed
Open
@eriawan
Copy link
Member Author

eriawan commented Oct 23, 2024

Looking at these failing checks, I think it's not caused by my PRs.

image

Especially this failed CI (of a test? ) on Regex:

image

@eriawan eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from b7a69c9 to 28c4b52 Compare October 31, 2024 20:36
@eriawan
Copy link
Member Author

eriawan commented Oct 31, 2024

rebase again with the latest main branch.

@dotnet/area-system-io team, could you please review this PR? It's been a month without any updates...

cc @jozkee @adamsitnik

@build-analysis build-analysis bot mentioned this pull request Oct 31, 2024
Open
@eriawan
Set encoding param in StreamReader ctor to be nullable
8dd5a9a
@eriawan
Set additional Encoding param to be nullable and refactor ValidateArg…
5b97654 
…sAndOpenPath to not require Encoding param anymore like StreamWriter
@eriawan
Set additional Encoding param to be nullable and refactor ValidateArg…
27773b6 
…sAndOpenPath to not require Encoding param anymore like StreamWriter
@eriawan eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from 28c4b52 to 305cd6b Compare November 3, 2024 17:48
@eriawan
Copy link
Member Author

eriawan commented Nov 3, 2024

rebase again with main branch as of 4th Nov.

stephentoub
stephentoub reviewed Nov 8, 2024
View reviewed changes
src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReader.StringCtorTests.cs
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamReader("path", (Encoding)null));
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamReader("path", null, true));
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamReader("path", null, true, null));
AssertExtensions.Throws<ArgumentNullException>("encoding", () => new StreamReader("path", null, true, -1));
Copy link
Member

Choose a reason for hiding this comment

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

Do we have tests that pass in null successfully?

Copy link
Member Author

@eriawan eriawan Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@stephentoub
You're right! I'm going to add these tests that focusing on null value passing today.
Hopefully I will submit the tests tomorrow the latest.

Thanks! BTW, I might be delayed as I'm also going to watch dotnetConf. 😊
Congratulations for the dotnetConf event and the launch of .NET 9.0! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephentoub stephentoub stephentoub left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StreamReader constructor documentation mismatch with implementation
4 participants
@eriawan @lilinus @rameel @stephentoub