Skip to content
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.

Sign up for GitHub

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

Jump to bottom

System.Text.Json based formatters #8362

Merged
merged 6 commits into from
Mar 21, 2019
Merged

System.Text.Json based formatters #8362

merged 6 commits into from
Mar 21, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Mar 9, 2019 •
edited
Loading

Fixes #7256

@pranavkm pranavkm requested a review from rynowak March 9, 2019 01:56
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 10, 2019
@pranavkm pranavkm mentioned this pull request Mar 13, 2019
Closed
@pranavkm pranavkm force-pushed the prkrishn/formatters branch 4 times, most recently from c0bd648 to 4fe623a Compare March 14, 2019 04:57
@pranavkm pranavkm marked this pull request as ready for review March 14, 2019 04:57
@pranavkm
Copy link
Contributor Author

🆙 📅

ahsonkhan
ahsonkhan reviewed Mar 20, 2019
View reviewed changes
src/Mvc/Mvc.Core/src/Formatters/TranscodingWriteStream.cs

if (buffer.Length - offset < count)
{
throw new ArgumentOutOfRangeException(nameof(count));
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why did you decide on throwing with count? What if offset > buffer.Length and count = 0?

@pranavkm pranavkm force-pushed the prkrishn/formatters branch from bdc60bf to 377f408 Compare March 20, 2019 23:23
@pranavkm pranavkm force-pushed the prkrishn/formatters branch from 377f408 to 9cf27be Compare March 21, 2019 16:50
@pranavkm pranavkm merged commit f5ff181 into master Mar 21, 2019
@pranavkm pranavkm deleted the prkrishn/formatters branch March 21, 2019 20:45
@jozkee jozkee mentioned this pull request Mar 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkotalik jkotalik jkotalik left review comments

@ahsonkhan ahsonkhan ahsonkhan left review comments

@stephentoub stephentoub stephentoub left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@rynowak rynowak rynowak approved these changes

@blowdart blowdart blowdart left review comments

@JamesNK JamesNK JamesNK left review comments

@davidfowl davidfowl davidfowl left review comments

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@pranavkm @davidfowl @JamesNK @blowdart @rynowak @GrabYourPitchforks @stephentoub @ahsonkhan @jkotalik @Eilon