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

Improve the handling of Outcome in V8 #1328

Closed
martintmk opened this issue Jun 20, 2023 · 4 comments
Closed

Improve the handling of Outcome in V8 #1328

martintmk opened this issue Jun 20, 2023 · 4 comments
Labels
v8 Issues related to the new version 8 of the Polly library.
Milestone

Comments

@martintmk
Copy link
Contributor

Currently, the Outcome<T> is created as:

var outcome = new Outcome<HttpResponseMessage>(new HttpResponseMessage(HttpStatusCode.Ok));

In Polly V8 there are many places where the delegate need a function that returns ValueTask<Outcome>:

var task = new ValueTask<Outcome<HttpResponseMessage>>(new Outcome<HttpResponseMessage>(new HttpResponseMessage(HttpStatusCode.Ok)));

The syntax gets a little obscure so I am thinking how we can simplify this handling.

How about introduce the Outcome static class that will be used to create instances of Outcome<T> ?

public static class Outcome
{
    public static Outcome<TResult> Result<TResult>(TResult value) => new(value);

    public static ValueTask<Outcome<TResult>> ResultTask<TResult>(TResult value) => new(Result(value));

    public static Outcome<TResult> Exception<TResult>(Exception exception) => new(exception);

    public static ValueTask<Outcome<TResult>> ExceptionTask<TResult>(Exception exception) => new(Exception<TResult>(exception));
}

Now the usage becomes:

var outcome = Outcome.Result(new HttpResponseMessage(HttpStatusCode.Ok));
var task = Outcome.ResultTask(new HttpResponseMessage(HttpStatusCode.Ok));

We can take advantage of automatic type resolution. The constructors on Outcome<T> would be hidden and using the Outcome static class would be the only way to create instances of generic outcome.

This could also clean-up the V8 code a bit.

Contributes to #1324 and #1301.

@martincostello , @joelhulen, @PeterCsalaHbo Your thoughts, should I give this a shot?

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Jun 20, 2023
@martintmk martintmk added this to the v8.0.0 milestone Jun 20, 2023
@martincostello
Copy link
Member

Maybe call it FromResult(), like Task.FromResult()? Otherwise seems reasonable to me.

@martintmk
Copy link
Contributor Author

@martincostello Any suggestions for the method that returns ValueTask based outcome? I don't want to put Async suffix there because then the analyzers recommend it when it's not relevant?

How about this?

public static ValueTask<Outcome<TResult>> FromResultAsTask<TResult>(TResult value) => new(Result(value));

Or maybe allow this syntax?

Outcome.FromResult("some-value").AsValueTask();

@martincostello
Copy link
Member

FromResultAsTask is the best option, I think.

@PeterCsalaHbo
Copy link

For me the FromResult and FromResultAsTask seem fine.

public static class Outcome
{
    public static Outcome<TResult> FromResult<TResult>(TResult value) => ...;
    public static ValueTask<Outcome<TResult>> FromResultAsTask<TResult>(TResult value) => ...;
    public static Outcome<TResult> FromException<TResult>(Exception exception) => ...;
    public static ValueTask<Outcome<TResult>> FromExceptionAsTask<TResult>(Exception exception) => ...;
}

I think the AsValueTask would make sense if some API would return an Outcome<TResult> but you need to wrap it into a ValueTask to be compatible with other APIs. But as you said the V8 is async by nature so, if there is no API which returns Outcome<TResult> then having a single method, which combines the Outcome and ValueTask wrappings, eases the usage IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants