-
Notifications
You must be signed in to change notification settings - Fork 304
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
Features/combine sequential #280
base: master
Are you sure you want to change the base?
Features/combine sequential #280
Conversation
…lExtensions into features/combine-sequential
I would like to get a feedback. If you approve the idea, I will implement the method for more parameters |
Func<Task<Result<TDataB>>> b, | ||
Func<(TDataA DataA, TDataB DataB), TReturnData> combineFunction) | ||
=> await (await a()) | ||
.Bind(async resultA => (await b()).Map(resultB => combineFunction((resultA, resultB)))); |
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.
Such one-lines are hard to read. Please decompose into several lines, such as:
{
var resultA = await a();
var resultB = await b();
// Bind, Map, etc
}
@@ -50,7 +50,16 @@ public static Maybe<T> Where<T>(this Maybe<T> maybe, Func<T, bool> predicate) | |||
|
|||
return Maybe<T>.None; | |||
} | |||
public static Result<K> Select<T, K>(this Result<T> maybe, Func<T, K> selector) |
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.
Please don't add Select
overloads, those should be either Map or Bind.
|
||
[Obsolete("Use Bind instead of this 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.
Same here, no need to add new obsolete methods
Task<Result<string>> SecondFunction() => Task.FromResult(Result.Success("value")); | ||
|
||
var result = await Result.CombineSequential(FirstFunction, | ||
SecondFunction, |
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.
Minor (nit): the indentation is off.
I've left a couple of comments. Otherwise, looks good, please proceed with the overloads. |
This is the first approach for CombineSequential.