Skip to content

Added Write/WriteLine ReadOnlySpan<char> override to HttpResponseStreamWriter #18445

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.

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

Closed

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