Skip to content

Conversation

alefranz
Copy link
Contributor

@alefranz alefranz commented Jan 18, 2020

Partially address #2895

/cc @benaadams

@alefranz
Copy link
Contributor Author

Tests failures should be unrelated

@jkotalik
Copy link
Contributor

I don't think this is the right issue number.

@@ -161,10 +161,12 @@ public partial class HttpResponseStreamWriter : System.IO.TextWriter
public override System.Threading.Tasks.Task FlushAsync() { throw null; }
public override void Write(char value) { }
public override void Write(char[] values, int index, int count) { }
public override void Write(System.ReadOnlySpan<char> values) { }
public override void Write(string value) { }
public override System.Threading.Tasks.Task WriteAsync(char value) { throw null; }
public override System.Threading.Tasks.Task WriteAsync(char[] values, int index, int count) { throw null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a parallel method for WriteAsync that accepts a ReadOnlyMemory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have #18451 for those, but it is based on this one, so I will mark it ready for review once this is merged. Unless you prefer to combine it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now!

}

int written = 0;
while (written < values.Length)
Copy link
Contributor

@jkotalik jkotalik Jan 21, 2020

Choose a reason for hiding this comment

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

nit: save values.Length in a local rather than checking every iteration of the loop. nvrm you are slicing it

@jkotalik jkotalik added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 21, 2020
@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Jan 21, 2020
Copy link
Contributor Author

@alefranz alefranz left a comment

Choose a reason for hiding this comment

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

@jkotalik Fixed issue number. Sorry about that.

@jkotalik
Copy link
Contributor

I think we should close this PR and just do #18451. We need to do an API review for this change as it adds new public API, and I'm not afraid of this change being too large.

@alefranz I'm going to close this for now and start reviewing your other PR. Feel free to reopen if you have any objections 😄

@jkotalik jkotalik closed this Jan 22, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants