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.

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

✨ Result Status Helpers #176

Closed
danielmackay opened this issue Apr 27, 2024 · 4 comments · Fixed by #184
Closed

✨ Result Status Helpers #176

danielmackay opened this issue Apr 27, 2024 · 4 comments · Fixed by #184

Comments

@danielmackay
Copy link
Contributor

Pain

When checking the result status (for example a service called from a minimal API). The code seems more verbose that it could be.

For example:

app
  .MapPost("/",
      async Task<Results<Created, BadRequest<ValidationProblemDetails>, NotFound>> (
          [FromServices] ISender sender,
          [FromBody] CreateLeaveCommand command, CancellationToken ct) =>
      {
          var result = await sender.Send(command, ct);
          if (result.Status == ResultStatus.BadRequest) // 👈 here
              return TypedResults.BadRequest(result.ToValidationProblem());
  
          if (result.Status == ResultStatus.NotFound) // 👈 here
              return TypedResults.NotFound();
  
          return TypedResults.Created();
      })
  .WithName("CreateLeave");

Solution

Add helper fields to IResult so that the code is more concise. The result would look something like:

app
  .MapPost("/",
      async Task<Results<Created, BadRequest<ValidationProblemDetails>, NotFound>> (
          [FromServices] ISender sender,
          [FromBody] CreateLeaveCommand command, CancellationToken ct) =>
      {
          var result = await sender.Send(command, ct);
          if (result.IsInvalid) // 👈 here
              return TypedResults.BadRequest(result.ToValidationProblem());
  
          if (result.IsNotFound) // 👈 here
              return TypedResults.NotFound();
  
          return TypedResults.Created();
      })
  .WithName("CreateLeave");

The above reads much cleaner and removes unneeded code.

@ardalis:

  1. What do you think of the above?
  2. Would you be happy for me to create a PR for this?
  3. Do you think IResult is the best place to put this?
@ardalis
Copy link
Owner

ardalis commented May 16, 2024

  1. I like it.
  2. Yes!
  3. I'm not sure whether IResult or Result is better. The only thing that makes me unsure is that IResult has been co-opted by the dotnet team(https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult?view=aspnetcore-8.0), so there might be confusion with its usage (either by developers or by the compiler or both).

If you don't see an issue with the IResult type collision, then let's try that.

If you opt to use IResult, create a new class IResultExtensions for the new methods. Otherwise just add them to the existing ResultExtensions type.

Confirm you're still interested in the PR and I'll assign this to you (or just submit the PR if that's quick).

Thanks!

@sunecko
Copy link
Contributor

sunecko commented May 17, 2024

@ardalis @danielmackay This could be solved by adding the methods to ResultExtensions class?

public static bool IsUnauthorized(this Result result)
    => result.Status == ResultStatus.Unauthorized;
public static bool IsForbidden(this Result result)
     => result.Status == ResultStatus.Forbidden;

These two are examples, if you think it is the correct way to do it I would be happy to make my contribution.

@danielmackay
Copy link
Contributor Author

  1. I like it.
  2. Yes!
  3. I'm not sure whether IResult or Result is better. The only thing that makes me unsure is that IResult has been co-opted by the dotnet team(https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult?view=aspnetcore-8.0), so there might be confusion with its usage (either by developers or by the compiler or both).

If you don't see an issue with the IResult type collision, then let's try that.

If you opt to use IResult, create a new class IResultExtensions for the new methods. Otherwise just add them to the existing ResultExtensions type.

Confirm you're still interested in the PR and I'll assign this to you (or just submit the PR if that's quick).

Thanks!

Hey @ardalis. Thanks for the reply! Yes I am still interesting in actioning this PR. Please assign to me and I will aim to get it done over the next week.

Cheers!

@danielmackay
Copy link
Contributor Author

@ardalis I ended up opting for the extension methods to be agains IResult as that is the lowest common denominator. I don't believe there will be a problem with the type collision.

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 a pull request may close this issue.

3 participants