-
Notifications
You must be signed in to change notification settings - Fork 136
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
Added convenience methods for easier Monad creation and Monad chaining #258
base: develop
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree |
@julianthurner , thanks for your contribution! According to the Contribution Guideline, could you change the target branch from |
src/DotNext/Result.cs
Outdated
/// <typeparam name="T">The type of the value.</typeparam> | ||
/// <typeparam name="TResult">The type of the result of the mapping function.</typeparam> | ||
/// <returns>The conversion result.</returns> | ||
public static async Task<Result<TResult>> Convert<T, TResult>(this Task<Result<T>> task, Converter<T, TResult> converter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task<Result<T>>
is a subject for debates, because Task
is already a monad that contains a result or an exception. Both types have the similar semantics. Thus, Task<Result<T>>
is the same as Result<Result<T>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that point, however: If i have an async
method which contains one ore more await
statements, then the result of the method has to be Task<T>
. And as far as I am concerned, there is no implicit conversion from Task<T>
to Result<T>
. Or did I overlook that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some methods to convert task types located in Conversion class. Also, there is a method
static AwaitableResult<T> SuspendException<T>(this Task<T> task)
not yet released. With help of these methods, you can create async pipeline as follows:
Result<double> result = await SomeAsyncMethod().Convert<int, double>(i => i).SuspendException();
It's called SuspendException
because the returned awaitable object never throws. Instead, the exception is returned as the part of Result<T>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AwaitableResult seems like a neat feature!
Will this also be chainable like so?
var result = await Task.FromResult(42).SuspendException()
.Convert(async x => x > 10 ? x * 2.0 : throw new Exception("x too small")).SuspendException()
.Convert(async x => x > 42 ? x.ToString() : throw new Exception("x less than 42").SuspendException();
If not, I can wait until the method is implemented and then add the respective extension methods that are needed to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SuspendException
is the last call in the chain since the Task
is already acts as a monad:
Result<int> result = await Task.FromResult(42)
.Convert(async x => x > 10 ? x * 2.0 : throw new Exception("x too small"))
.Convert(async x => x > 42 ? x.ToString() : throw new Exception("x less than 42")
.SuspendException();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Then I'll remove the commented method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. SuspendException
is available in develop
branch (added in 0cb6a6a)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sakno could you take a look at the updated branch? I changed all methods to work with AwaitableResult
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SuspendException
is a tail call, no need apply any transformation to it. As shown above, all transformations can be done before SuspendException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SuspendException
is a tail call, no need apply any transformation to it.
What part of the code are you referring to / what do you suggest I should change?
Changing the target branch is not enough. Also, after the discussion of the API, units tests need to be added as well. |
/// <param name="task">The task containing Optional value.</param> | ||
/// <param name="converter">A mapping function to be applied to the value, if present.</param> | ||
/// <returns>An Optional describing the result of applying a mapping function to the value of this Optional, if a value is present, otherwise <see cref="Optional{T}.None"/>.</returns> | ||
public static async Task<Optional<TOutput>> Convert<TInput, TOutput>(this Task<Optional<TInput>> task, Converter<TInput, Task<Optional<TOutput>>> converter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is also redundant, because Convert
is already defined for generic Task<T>
in DotNext.Threading.Tasks.Conversion
static class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me a while to find time again, I think this method is not completely redundant because omitting it means having to handle the Optional
yourself every time instead of being able to rely on the monad chain handling it which defeats the whole purpose of the chain (see examples below). It also means that if I use static conversion methods which I might re-use elsewhere in the code instead of lambdas, I have to force Optional<TInput>
as input type instead of being able to simply accept TInput
.
Without extension method:
var optional = await Task.FromResult(Optional.FromValue(42))
.Convert(async x => x > 10 ? x * 2.0 - 20 : Optional<double>.None)
.Convert(async x => x > 0 ? "Success" : Optional<string>.None);
With extension method:
var optional = await Task.FromResult(Optional.From(42))
.Convert(async x => x.HasValue && x.Value > 10 ? x.Value * 2.0 - 20 : Optional<double>.None)
.Convert(async x => x.HasValue && x.Value > 0 ? "Success" : Optional<string>.None);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed method has no cancellation token support. Since 5.19.0 that is just released, there is Convert
with it:
Task<Optional<TOutput>> Convert<TInput, TOutput>(this Task<Optional<TInput>> task,
Func<TInput, CancellationToken, Task<TOutput>> converter, CancellationToken token = default)
fe42f95
to
e1ee1f1
Compare
e1ee1f1
to
f58043f
Compare
I would suggest to review the proposed API again from your side, since 5.19.0 has been released:
|
Looks nice👍I will review the changes and adjust them accordingly, sometime this week probably. |
I added some convenience methods to the library. The Option class receives a few extension methods that allow for chaining async conversions:
This is really useful if you work with databases where data is fetched / transformed multiple times asynchronously before being filled into a DTO.
The same thing for Result:
This also forwards the exception up the chain just like with regular monads.
There's also overloads for converting Option ↔ Result in both directions if there's need for conversion within a chain.
Also, I added some additional convenience methods for creating Options and Results statically: