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

eriawan · 2024-10-04T05:03:29Z

dotnet-issue-labeler · 2024-10-04T05:03:35Z

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-issue-labeler · 2024-10-04T05:03:37Z

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.

eriawan · 2024-10-04T05:05:35Z

@jozkee please review

cc @rameel

lilinus · 2024-10-04T14:00:13Z

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 · 2024-10-04T14:32:03Z

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 · 2024-10-05T03:59:15Z

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 · 2024-10-05T20:43:37Z

@lilinus @rameel

Done updating the ccror with the string path as well.

@jozkee
Could you please review?

eriawan · 2024-10-05T21:42:44Z

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

eriawan · 2024-10-10T08:34:24Z

@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 · 2024-10-11T07:14:44Z

@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 · 2024-10-11T07:23:49Z

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 · 2024-10-11T07:36:44Z

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 · 2024-10-22T20:17:45Z

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 · 2024-10-22T20:30:24Z

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

eriawan · 2024-10-23T16:42:16Z

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

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

eriawan · 2024-10-31T20:39:00Z

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

eriawan · 2024-11-03T17:49:31Z

rebase again with main branch as of 4th Nov.

src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReader.StringCtorTests.cs

eriawan · 2024-12-19T16:47:07Z

Adding more unit tests to test when calling ctor that has null Encoding should not throw exception.

danmoseley · 2024-12-19T18:23:57Z

src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReader.StringCtorTests.cs

+            var ctorParamStreamEncodingBoolIntEx = Record.Exception(ctorParamStreamEncodingBoolInt);
+            Assert.Null(ctorParamStreamEncodingBoolIntEx);
+
+            Action ctorParamStreamEncodingBoolIntBool = () => new StreamReader(new MemoryStream(), null, false, 100, false);


Can't you just call the ctor? Any exception will fail the test.

Thanks! Done updating the test and call the constructor instead as suggested.
Please review, @danmoseley

Tests look fine, I'll let someone in the area sign off though.

@danmoseley
thanks! 👍

eriawan · 2024-12-22T14:34:34Z

Rebase again as of 22nd Dec of main branch.

Could someone from .NET team review this PR?

@dotnet/area-system-io , @adamsitnik , @jozkee
cc @danmoseley

…sAndOpenPath to not require Encoding param anymore like StreamWriter

…as been set to nullable

… not throw Exception.

…. Also just call the ctor directly as suggested in PR feedback.

eriawan · 2025-01-09T21:10:45Z

Rebase again, against main branch as of 1/10/2025.

@danmoseley Is there anything else I should do to get this PR reviewed and merged?
I hope this PR will be approved and then be merged soon, as it has been quite long (more than a month) without any further review/approval.

cc @dotnet/area-system-io , @jozkee @adamsitnik

eriawan · 2025-01-09T22:01:43Z

Hmm... those failed builds of runtime (Build ios-arm64 Release AllSubsets_Mono), runtime (Build ios-arm64 Release AllSubsets_NativeAOT), untime (Build osx-x64 Release NativeAOT) and runtime (Build tvossimulator-x64 Debug AllSubsets_Mono) are not related to my changes at all, because these failed builds were ok before.

adamsitnik

LGTM, thank you for your contribution @eriawan and apologies for the delay in reviews!

adamsitnik · 2025-01-10T16:21:18Z

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

@@ -187,18 +187,18 @@ public StreamReader(string path, bool detectEncodingFromByteOrderMarks)
        {
        }

-        public StreamReader(string path, Encoding encoding)
+        public StreamReader(string path, Encoding? encoding)


I was suprised to see this change, as it was not approved as part of #106236 (comment).

But then I've found @jozkee comment #106658 (review) referring to #2376 (comment) that explains that we need to unify all ctors that accept Encoding. Which we already did for StreamWriter in #106658.

So everything is good 👍

eriawan · 2025-01-10T19:43:50Z

LGTM, thank you for your contribution @eriawan and apologies for the delay in reviews!

Yayyyyy!!! Many thanks for the review, approval and the merge, @adamsitnik ! 👍

dotnet-issue-labeler bot added area-System.IO new-api-needs-documentation labels Oct 4, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 4, 2024

build-analysis bot mentioned this pull request Oct 4, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

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

eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from c74bd44 to b7a69c9 Compare October 22, 2024 20:29

This was referenced Oct 23, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

WasmTestOnChrome timeouts in CI #105363

Closed

System.Net.Http.Functional.Tests.TelemetryTest failing with differing string #109024

Open

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

build-analysis bot mentioned this pull request Oct 31, 2024

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Open

eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from 28c4b52 to 305cd6b Compare November 3, 2024 17:48

stephentoub reviewed Nov 8, 2024

View reviewed changes

src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReader.StringCtorTests.cs Show resolved Hide resolved

eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from 305cd6b to d928bea Compare December 16, 2024 15:12

danmoseley reviewed Dec 19, 2024

View reviewed changes

build-analysis bot mentioned this pull request Dec 19, 2024

The hosted runner encountered an error while running your job. (Error Type: Disconnect). dotnet/dnceng#1919

Open

3 tasks

eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from ccfa9b7 to ba7b7a5 Compare December 22, 2024 14:32

build-analysis bot mentioned this pull request Dec 22, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

eriawan added 7 commits January 10, 2025 03:13

Set encoding param in StreamReader ctor to be nullable

b0ed7bd

Set additional Encoding param to be nullable and refactor ValidateArg…

b3761ed

…sAndOpenPath to not require Encoding param anymore like StreamWriter

Set additional Encoding param to be nullable and refactor ValidateArg…

f153b00

…sAndOpenPath to not require Encoding param anymore like StreamWriter

Refactor StreamReader_StringCtorTests accordingly as Encoding param h…

bf87d04

…as been set to nullable

Add additional test to check calling ctor with Encoding = null should…

ad047bb

… not throw Exception.

Use empty string instead of path

c2da02a

Remove all tests related to path usage, focus on usint Stream instead…

c4ad463

…. Also just call the ctor directly as suggested in PR feedback.

eriawan force-pushed the streamreader_ctor_encoding_to_nullable branch from ba7b7a5 to c4ad463 Compare January 9, 2025 21:00

This was referenced Jan 10, 2025

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

ModuleNotFoundError: No module named 'pkg_resources' dotnet/dnceng#4756

Open

adamsitnik approved these changes Jan 10, 2025

View reviewed changes

adamsitnik merged commit 5abf582 into dotnet:main Jan 10, 2025
132 of 139 checks passed

adamsitnik added this to the 10.0.0 milestone Jan 10, 2025

adamsitnik self-assigned this Jan 10, 2025

eriawan deleted the streamreader_ctor_encoding_to_nullable branch January 10, 2025 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

eriawan commented Oct 4, 2024

dotnet-issue-labeler bot commented Oct 4, 2024

dotnet-issue-labeler bot commented Oct 4, 2024

eriawan commented Oct 4, 2024

lilinus commented Oct 4, 2024

rameel commented Oct 4, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 10, 2024

eriawan commented Oct 11, 2024

lilinus commented Oct 11, 2024

eriawan commented Oct 11, 2024

eriawan commented Oct 22, 2024

eriawan commented Oct 22, 2024

eriawan commented Oct 23, 2024

eriawan commented Oct 31, 2024

eriawan commented Nov 3, 2024

eriawan commented Dec 19, 2024

danmoseley Dec 19, 2024

eriawan Dec 19, 2024

danmoseley Dec 20, 2024

eriawan Dec 22, 2024

eriawan commented Dec 22, 2024

eriawan commented Jan 9, 2025

eriawan commented Jan 9, 2025

adamsitnik left a comment

adamsitnik Jan 10, 2025

eriawan commented Jan 10, 2025

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

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

Conversation

eriawan commented Oct 4, 2024

dotnet-issue-labeler bot commented Oct 4, 2024

dotnet-issue-labeler bot commented Oct 4, 2024

eriawan commented Oct 4, 2024

lilinus commented Oct 4, 2024

rameel commented Oct 4, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 5, 2024

eriawan commented Oct 10, 2024

eriawan commented Oct 11, 2024

lilinus commented Oct 11, 2024

eriawan commented Oct 11, 2024

eriawan commented Oct 22, 2024

eriawan commented Oct 22, 2024

eriawan commented Oct 23, 2024

eriawan commented Oct 31, 2024

eriawan commented Nov 3, 2024

eriawan commented Dec 19, 2024

danmoseley Dec 19, 2024

Choose a reason for hiding this comment

eriawan Dec 19, 2024

Choose a reason for hiding this comment

danmoseley Dec 20, 2024

Choose a reason for hiding this comment

eriawan Dec 22, 2024

Choose a reason for hiding this comment

eriawan commented Dec 22, 2024

eriawan commented Jan 9, 2025

eriawan commented Jan 9, 2025

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik Jan 10, 2025

Choose a reason for hiding this comment

eriawan commented Jan 10, 2025