Skip to content

[Codefixer] Recommend using TypedResults over using Results #45217

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

Open
1 of 15 tasks
captainsafia opened this issue Nov 21, 2022 · 3 comments
Open
1 of 15 tasks

[Codefixer] Recommend using TypedResults over using Results #45217

captainsafia opened this issue Nov 21, 2022 · 3 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Nov 21, 2022

Background and Motivation

  • Recommend returning TypedResults instead of Results
  • Recommend adding the Results<> return type to your class
app.MapGet("/", (int id) => SomeCondition(id) ? Results.Ok() : Results.NotFound());

Becomes

app.MapGet("/", (int id) => SomeCondition(id) ? TypedResults.Ok() : TypedResults.NotFound());

And further becomes:

app.MapGet("/", Results<Ok, NotFound>(int id) => SomeCondition(id) ? TypedResults.Ok() : TypedResults.NotFound());

Proposed Analyzer

Analyzer Behavior and Message

Category

  • Design
  • Documentation
  • Globalization
  • Interoperability
  • Maintainability
  • Naming
  • Performance
  • Reliability
  • Security
  • Style
  • Usage

Severity Level

  • Error
  • Warning
  • Info
  • Hidden

Usage Scenarios

Risks

@captainsafia captainsafia added api-suggestion Early API idea and discussion, it is NOT ready for implementation analyzer Indicates an issue which is related to analyzer experience labels Nov 21, 2022
@mkArtakMSFT mkArtakMSFT added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jan 26, 2023
@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed analyzer Indicates an issue which is related to analyzer experience old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@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 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@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 6, 2024
@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 dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@captainsafia captainsafia added this to the Backlog milestone Apr 30, 2024
@captainsafia
Copy link
Member Author

Cleaning things out and moving this to the backlog. I think this should be done in two parts: an analyzer to recommend TypedResults over Results and an analyzer to recommend adding the results type. IMO, the second one is probably less urgent.

Paging resident analyzer guru @david-acker for any interest/time in doing this.

@david-acker
Copy link
Member

@captainsafia I'd be happy to pick this up when I have some free time!

@gregatwisetech
Copy link

@david-acker @captainsafia

I've come across this issue while searching for whether an analyzer exists for it.

Below, "part 1" and "part 2" refer to parts as suggested by @captainsafia.

Here's my thinking, which I think is very close to part 2: I would like to analyze IEndpointRouteBuilder.Map...() calls and suggest using Results<> if the delegate passed in returns an IResult.

The reason I would like to implement part 2 is that my use case is that I want to prevent returning IResult.

If I start with part 1, that is not sufficient to reach my goal - when TypedResults are used, the return type of the delegate may still be IResult (it will be the right type in the case of the example in the OP, but it won't for methods returning IResult being passed in).

If I start with part 2 though, it will implicitly take care of the first part as well: since Results<> is a more specific type than IResult, calls to Results methods will result in a CS0266 compiler error (Cannot implicitly convert type 'Microsoft.AspNetCore.Http.IResult' to 'Microsoft.AspNetCore.Http.HttpResults.Results...') thus providing an indirect prompt to use TypedResults.

Time permitting, I may opt for implementing both parts, but part 2 is the one I need.

Since it's not tagged with help wanted, please let me know if you would be interested in receiving this contribution and if you have any feedback regarding the approach I suggested, as well as any suggestions for the wording of the diagnostic. I need this analyzer either way and I'd be happy to share it.

Please note I currently do not intend to provide a code fix for it, just the analyzer.

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-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
None yet
Development

No branches or pull requests

6 participants