Override TextWriter.Write{Line}{Async} on StringWriter by MarcoRossignoli · Pull Request #30667 · dotnet/corefx (original) (raw)
This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Conversation30 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| #region Task based Async APIs |
|---|
| public override void WriteLine(StringBuilder value) |
| { |
| _sb.Append(value); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the newline. As written, it's behaving the same as Write.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sorry
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| return Task.FromCanceled(cancellationToken); |
|---|
| } |
| _sb.Append(value); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about missing a newline.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sorry
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -178,8 +183,13 @@ public override void WriteLine(ReadOnlySpan buffer) |
|---|
| WriteLine(); |
| } |
| #region Task based Async APIs |
| public override void WriteLine(StringBuilder value) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these overloads should have a check like the one here:
https://github.com/dotnet/corefx/pull/30667/files#diff-062fa053ba9ce2bdd08a5926cfd4fccaR169
The problem is that someone may have already derived from StringWriter and overridden the core methods to do something different. If we then don't delegate to those from this new overload, we could start doing something wrong when the new method is used on their derived type. Since we don't have an efficient way to check for that specific condition, we instead just check whether this is a concrete StringWriter or something derived from it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clear!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -178,8 +183,13 @@ public override void WriteLine(ReadOnlySpan buffer) |
|---|
| WriteLine(); |
| } |
| #region Task based Async APIs |
| public override void WriteLine(StringBuilder value) |
| { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Override TextWriter.Write{Line}{Async} on StringWriter [WIP]Override TextWriter.Write{Line}{Async} on StringWriter
| } |
|---|
| _sb.Append(value); |
| WriteLine(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why not _sb.AppendLine()
Are you asking why StringBuilder doesn't have an AppendLine(StringBuilder) method? Simply because it'd be a rare need.
Even if it existed, though, you wouldn't want to use it here, as TextWriter let's you change what the NewLine characters to be used are, and StringBuilder's AppendLine wouldn't respect that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it existed, though, you wouldn't want to use it here, as TextWriter let's you change what the NewLine characters to be used are, and StringBuilder's AppendLine wouldn't respect that.
Understood!
[WIP]Override TextWriter.Write{Line}{Async} on StringWriter Override TextWriter.Write{Line}{Async} on StringWriter
Tests?
@stephentoub i've added tests.
UWP NETNative x86 Release Build — Build finished.
13:52:20 System\IO\StringWriter.cs(162,30): error CS0115: 'StringWriter.Write(StringBuilder)': no suitable method found to override [D:\j\workspace\windows-TGrou---c60886e1\src\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj]
If i understood well some TargetGroup doesn't provide StringBuilder overload. If so what is the correct way to solve(#ifdef, partial class with conditions...)?
| StringWriter sw = new StringWriter(); |
|---|
| CancellationTokenSource cts = new CancellationTokenSource(); |
| cts.Cancel(); |
| await Assert.ThrowsAsync(async () => await sw.WriteAsync(sb, cts.Token)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will pass even if the cancellation occurs asynchronously. It'd be better to write it as something like:
Assert.Equal(TaskStatus.Canceled, sw.WriteAsync(sb, cts.Token).Status);
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i amended code...but it's not clear to me why. Can you explain?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear to me why. Can you explain?
The version you had would pass whether the implementation completed with the exception synchronously or whether the implementation completed with the exception asynchronously. In this case, since the cancellation occurred prior to the method being called, we want it to always complete in this case synchronously, and so we want to verify that as best we can, which my suggested version does.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhhh yes...understood...too smart for me 😅
| StringWriter sw = new StringWriter(); |
|---|
| CancellationTokenSource cts = new CancellationTokenSource(); |
| cts.Cancel(); |
| await Assert.ThrowsAsync(async () => await sw.WriteLineAsync(sb, cts.Token)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| { |
|---|
| if (GetType() != typeof(StringWriter)) |
| { |
| // This overload was added after the Write(StringBuilder, ...) overload, and so in case |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"after the Write(StringBuilder, ...) overload" =>
"after the Write(char[], ...) overload"
| { |
|---|
| if (GetType() != typeof(StringWriter)) |
| { |
| // This overload was added after the WriteLine(StringBuilder, ...) overload, and so in case |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit
| { |
|---|
| if (GetType() != typeof(StringWriter)) |
| { |
| // This overload was added after the WriteAsync(StringBuilder, ...) overload, and so in case |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit
| { |
|---|
| if (GetType() != typeof(StringWriter)) |
| { |
| // This overload was added after the WriteLineAsync(StringBuilder, ...) overload, and so in case |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nit
UWP NETNative x86 Release Build — Build finished.
13:52:20 System\IO\StringWriter.cs(162,30): error CS0115: 'StringWriter.Write(StringBuilder)': no suitable method found to override [D:\j\workspace\windows-TGrou---c60886e1\src\System.Runtime.Extensions\src\System.Runtime.Extensions.csproj]
@stephentoub i see that CI now it's ok...is for this reason #30447 (comment)?
Is there a way to understand it by myself(for future, i've been thinking it's my fault, some docs, issues, PR around)?
is for this reason
Yes, the Corelib changes hadn't yet propagated to the .NET Native bits.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
picenka21 pushed a commit to picenka21/runtime that referenced this pull request
add overloads
fix overloads
add tests
move test to StringWriterTests.netcoreapp.cs
revert StringWriterTests.cs updates
address PR feedback
fix test
amend comments
Commit migrated from dotnet/corefx@57bb0fc