Skip to content

Conversation

@ekaradev
Copy link
Contributor

@ekaradev ekaradev commented Jun 18, 2021

Hi @fabiocav , @ankitkumarr
Can you review it when you have time ?
Resolves #344

@fabiocav
Copy link
Member

@emrekara37 thank you for the PR!

As is, this PR introduces a behavior change that could be breaking for customer relying on the existing functionality (typically, these kinds of behavior changes are not made in a minor version).

An option to address this is to introduce an overload that takes the status code as an additional argument, keeping the default as-is for backwards compatibility. Thoughts?

@ekaradev
Copy link
Contributor Author

@emrekara37 thank you for the PR!

As is, this PR introduces a behavior change that could be breaking for customer relying on the existing functionality (typically, these kinds of behavior changes are not made in a minor version).

An option to address this is to introduce an overload that takes the status code as an additional argument, keeping the default as-is for backwards compatibility. Thoughts?

Hi @fabiocav

i think you're right, i can implement overload version if you want

@fabiocav
Copy link
Member

@emrekara37 if you're open to that, absolutely! That would be great!

@ekaradev
Copy link
Contributor Author

@emrekara37 if you're open to that, absolutely! That would be great!

I updated the PR

Copy link
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Thank you!

Small nit comments.

@ghost ghost removed the Needs: Author Feedback label Jul 1, 2021
@ekaradev
Copy link
Contributor Author

ekaradev commented Jul 7, 2021

Thank you!

Small nit comments.

Hi @fabiocav,
i updated the PR

@fabiocav
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fabiocav fabiocav merged commit c9c5256 into Azure:main Jul 21, 2021
@fabiocav
Copy link
Member

Thank you!

@haacked
Copy link

haacked commented Aug 6, 2021

Thanks for resolving #344! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpResponseDataExtensions.WriteAsJsonAsync overwrites response status code

3 participants