Skip to content

Added InternalServerError and InternalServerError<TValue> to TypedResults #53656

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
Merged
merged 6 commits into from
Mar 4, 2024
Merged

Added InternalServerError and InternalServerError<TValue> to TypedResults #53656

merged 6 commits into from
Mar 4, 2024

Conversation

onurmicoogullari
Copy link
Contributor

@onurmicoogullari onurmicoogullari commented Jan 26, 2024 •
edited
Loading

Extending TypedResults with InternalServerError

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Added InternalServerError and InternalServerError<TValue> to TypedResults.

Fixes #53073

@ghost ghost added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member labels Jan 26, 2024
@ghost
Copy link

ghost commented Jan 26, 2024

Thanks for your PR, @onurmicoogullari. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@onurmicoogullari onurmicoogullari mentioned this pull request Jan 26, 2024
Closed
@onurmicoogullari
Copy link
Contributor Author

@onurmicoogullari please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@davidfowl
Copy link
Member

davidfowl commented Jan 27, 2024 •
edited
Loading

APIs need to go through API review and have to be approved by the team. You can leave the PR here until the right shape gets approved.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 4, 2024
@onurmicoogullari onurmicoogullari force-pushed the onurmicoogullari/typedresults-internalservererror branch from 0312554 to bfec86b Compare February 4, 2024 20:10
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@dotnet dotnet deleted a comment Feb 13, 2024
@wtgodbe wtgodbe removed the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 13, 2024
@onurmicoogullari onurmicoogullari force-pushed the onurmicoogullari/typedresults-internalservererror branch from f3b76c9 to e5d093a Compare February 14, 2024 20:38
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 22, 2024
@onurmicoogullari onurmicoogullari force-pushed the onurmicoogullari/typedresults-internalservererror branch from e5d093a to 21aa0c6 Compare February 23, 2024 19:57
halter73
halter73 reviewed Feb 23, 2024
View reviewed changes
src/Http/Http.Results/src/Results.cs Outdated
/// </summary>
/// <param name="exception">The exception to be thrown.</param>
/// <returns>The created <see cref="IResult"/> for the response.</returns>
public static IResult InternalServerError(ExceptionDispatchInfo exception)
Copy link
Member

Choose a reason for hiding this comment

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

We just approved the API with everything except these ExceptionDispatchInfo overloads. I know it was our suggestion in the first place, so sorry to give you the runaround.

Thanks for sticking with this as we went through the API review process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@halter73 No problem at all. Does that mean that you want me to update the PR and remove the ExceptionDispatchInfo overload and related test coverage?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@onurmicoogullari onurmicoogullari force-pushed the onurmicoogullari/typedresults-internalservererror branch from 21aa0c6 to e98b366 Compare February 24, 2024 00:29
@halter73 halter73 removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 26, 2024
@amcasey amcasey merged commit 4cceeb1 into dotnet:main Mar 4, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview3 milestone Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@captainsafia captainsafia Awaiting requested review from captainsafia captainsafia is a code owner

@wtgodbe wtgodbe Awaiting requested review from wtgodbe

@JamesNK JamesNK Awaiting requested review from JamesNK

@mgravell mgravell Awaiting requested review from mgravell

Assignees

@adityamandaleeka adityamandaleeka

Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
9.0-preview3
Development

Successfully merging this pull request may close these issues.

Add InternalServerError and InternalServerError<T> to TypedResults
6 participants
@onurmicoogullari @davidfowl @halter73 @adityamandaleeka @amcasey @wtgodbe