Implement StreamOutputFormatter #1954
Conversation
ghost
added
the
| formatters, | ||
| ContentTypes); | ||
| } | ||
|
|
||
| var incomingAcceptHeaderMediaTypes = formatterContext.ActionContext.HttpContext.Request.GetTypedHeaders().Accept ?? |
There was a problem hiding this comment.
What's the implication/purpose of this change? seems unrelated
There was a problem hiding this comment.
Yes - It's unrelated, but not worth a separate PR. It was super unclear what happens when there is a single content, and lots of extra code was being executed. Since this is a scenario in this PR, I moved it up. Not extra tests needed
There was a problem hiding this comment.
Looking at the code now I see why you did the change. 👍 to clarity
|
updated |
| /// <inheritdoc /> | ||
| public async Task WriteAsync([NotNull] OutputFormatterContext context) | ||
| { | ||
| using (var valueAsStream = ((Stream)context.Object)) |
There was a problem hiding this comment.
Could we make this a Func<Stream>. This way we know we'll own the lifetime of the Stream and we wouldn't have to deal with streams being created but not disposed because the pipeline never got here..
There was a problem hiding this comment.
you mean like a pushstreamcontent, but how would the usage be...like you mean we should have a streamresult or something which takes this func?
There was a problem hiding this comment.
@pranavkm is suggesting that if the method returns Func<Stream> the formatter will identify it as well. We discussed it earlier, and this could be a nice enhancement (in a separate PR/issue).
Include Functional tests and unit tests Resolves #1653
yishaigalatzer
force-pushed
the
StreamFormatter
branch
from
0f6d407 to
31f1cbb
Compare
|
|
from me
|
Merged 5a3863d |
Include Functional tests and unit tests
Resolves #1653