-
Notifications
You must be signed in to change notification settings - Fork 310
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
With Extensions (Combine) #342
base: master
Are you sure you want to change the base?
Conversation
I removed the tests because I still have to think about a good way to test every overload without making a mess. |
Personally I feel it's better to use And, speaking honestly, I think those extensions do too much. It's hard to understand all those positional and generic parameters. Example from tests: await r1.WithMap(r2, (a, b) => Task.FromResult(a + b), (e1, e2) => "failure"); // I don't feel it's simple and easy to read/understand |
@@ -36,6 +36,10 @@ | |||
<ItemGroup Condition="'$(TargetFramework)'=='net40'"> | |||
<PackageReference Include="Microsoft.Bcl.Async" Version="1.0.168" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="$(TargetFramework.StartsWith('net4')) OR $(TargetFramework)=='netstandard2.0'"> | |||
<PackageReference Include="System.ValueTuple" Version="4.*" /> |
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.
System.ValueTuple
is shipped with the legacy framework starting from 4.7 and with netstandard starting from 2.0.
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.
Comparing TargetFramework
is considered leaky, for various good reasons, such as Mono RT, and "oh I missed a version", additionally we are not interested in the TargetFramework
at all, but in its features.
Preprocessor directives express features.
Instead, consider comparing using $(DefineConstants.Contains('NET5_0_OR_GREATER'))
which are supported by 3rd party runtimes and also well documented.
{ | ||
public partial class ResultExtensions | ||
{ | ||
public static Result<T, E2> BindError<T, E, E2>(this Result<T, E> self, |
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.
Doesn't OnFailureCompensate
cover such a case?
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 don't think so. OnFailureCompensate expects the same error type :) This maps from E
, to E2
{ | ||
public static partial class ResultExtensions | ||
{ | ||
public static Task<Result<TResult>> WithMap<T1, T2, TResult>(this Result<T1> a, |
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 feel it can be expressed via SelectMany
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.
If you're able to give an example, I'll be good with that :D
@@ -36,6 +36,10 @@ | |||
<ItemGroup Condition="'$(TargetFramework)'=='net40'"> | |||
<PackageReference Include="Microsoft.Bcl.Async" Version="1.0.168" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="$(TargetFramework.StartsWith('net4')) OR $(TargetFramework)=='netstandard2.0'"> | |||
<PackageReference Include="System.ValueTuple" Version="4.*" /> |
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.
Comparing TargetFramework
is considered leaky, for various good reasons, such as Mono RT, and "oh I missed a version", additionally we are not interested in the TargetFramework
at all, but in its features.
Preprocessor directives express features.
Instead, consider comparing using $(DefineConstants.Contains('NET5_0_OR_GREATER'))
which are supported by 3rd party runtimes and also well documented.
{ | ||
var mapSuccess = | ||
a.BindError(el1 => b | ||
.MapError(el2 => string.Join(Result.ErrorMessagesSeparator, el1, el2)) |
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.
Use Errors.Join
{ | ||
public static string Join(string e1, string e2) | ||
{ | ||
return string.Join(Result.ErrorMessagesSeparator, e1, e2); |
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.
Use string interpolation $"{e1}{Result.ErrorMessagesSeparator}{e2}"
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.
Would be sensible to wait for #378 with the error combining
{ | ||
public static partial class ResultExtensions | ||
{ | ||
public static Result<(T1, T2)> With<T1, T2>(this Result<T1> a, Result<T2> b) |
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.
Declare argument covariance in this
& in
for every non TAP, non closure usages accepting struct
s.
{ | ||
public static Result<(T1, T2)> With<T1, T2>(this Result<T1> a, Result<T2> b) | ||
{ | ||
return a.WithMap(b, (x, y) => (x, y)); |
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.
Use static () {}
lambda in all instances where no closure allocation is required.
These extensions will allow you combine Results.
This is done by combining successful values using a factory + merging errors using a factory
Example
combined will contain a success with a value of 4