-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add TaskValidation
module
#313
Conversation
tests/FsToolkit.ErrorHandling.Tests/FsToolkit.ErrorHandling.Tests.fsproj
Outdated
Show resolved
Hide resolved
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.
self-review complete
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.
Lots of great work here!
I left some comments on areas I think are missing.
Expect.equal actual (Validation.error errorMsg) "Should be Error" | ||
} | ||
|
||
testCaseTask "Fail Path Validation/Choice/AsyncValidation" |
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.
Unless I don't see it, I don't think there's tests covering seeing multiple failure cases. Something like how
testCase "Fail Path Result" |
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.
Added a test covering that scenario in latest commit. Let me know if that is what you were looking for.
|> Async.StartImmediateAsTask | ||
|
||
[<AutoOpen>] | ||
module TaskValidationCEExtensionsHighPriority2 = |
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 this is missing overloads for Result<'a,'err>
(where 'err
is not a list). You can see examples of the overload resolution in AsyncValidation:
module MediumPriority = |
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.
Added these overloads
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.
❤️ Thank you!
- BREAKING: [Remove Ply and update to FSharp 6](#248) Credits @TheAngryByrd - BREAKING: [Remove MergeSources (and!) from some implementations like Result](#261) Credits @TheAngryByrd - BREAKING: [Merge TaskResult into Core library](#285) Credits @TheAngryByrd - This means FsToolkit.ErrorHandling.TaskResult is no longer a separate package and will not be updated. It is now part of the core library. - BREAKING: [Rename retn to singleton](#287) Credits @1eyewonder - BREAKING: [Rename returnError to error + documentation](#311) Credits @tw0po1nt - [Use Microsoft.Bcl.AsyncInterfaces in netstandard2.0 (Allows IAsyncDisposable and IAsyncEnumerable)](#250) Credits @TheAngryByrd - [Build against Net8](#251) Credits @TheAngryByrd - [Fix Overload Resolution to Align to Computation Expression used](#252) Credits @TheAngryByrd - [refactor!: Seq.sequenceResultM returns Array instead of seq](#255) Credits @bartelink - [feat(Seq): sequenceResultA](#255) Credits @bartelink - [Updated uses of Seq.append](#290) Credits @1eyewonder - [Add Option.traverseAsync and Option.sequenceAsync](#298 (comment)) Credits @tw0po1nt - [Add Require and Check helper methods](#295) Credits @PI-Gorbo - [Add new AsyncOption APIs and document all its other functions; minor fixes to documentation for Option module](#307) Credits @tw0po1nt - [F# 9 support and nullness](#308) Credits @TheAngryByrd - [Update IcedTasks 0.11.7](0a4cc7b) Credits @TheAngryByrd - [Add TaskValidation module](#313) Credits @tw0po1n8
- BREAKING: [Remove Ply and update to FSharp 6](#248) Credits @TheAngryByrd - BREAKING: [Remove MergeSources (and!) from some implementations like Result](#261) Credits @TheAngryByrd - BREAKING: [Merge TaskResult into Core library](#285) Credits @TheAngryByrd - This means FsToolkit.ErrorHandling.TaskResult is no longer a separate package and will not be updated. It is now part of the core library. - BREAKING: [Rename retn to singleton](#287) Credits @1eyewonder - BREAKING: [Rename returnError to error + documentation](#311) Credits @tw0po1nt - [Use Microsoft.Bcl.AsyncInterfaces in netstandard2.0 (Allows IAsyncDisposable and IAsyncEnumerable)](#250) Credits @TheAngryByrd - [Build against Net8](#251) Credits @TheAngryByrd - [Fix Overload Resolution to Align to Computation Expression used](#252) Credits @TheAngryByrd - [refactor!: Seq.sequenceResultM returns Array instead of seq](#255) Credits @bartelink - [feat(Seq): sequenceResultA](#255) Credits @bartelink - [Updated uses of Seq.append](#290) Credits @1eyewonder - [Add Option.traverseAsync and Option.sequenceAsync](#298 (comment)) Credits @tw0po1nt - [Add Require and Check helper methods](#295) Credits @PI-Gorbo - [Add new AsyncOption APIs and document all its other functions; minor fixes to documentation for Option module](#307) Credits @tw0po1nt - [F# 9 support and nullness](#308) Credits @TheAngryByrd - [Update IcedTasks 0.11.7](0a4cc7b) Credits @TheAngryByrd - [Add TaskValidation module](#313) Credits @tw0po1nt - [feat(Seq.traverse/sequence*)!: Yield arrays](#310) Credits @bartelink
Proposed Changes
Add
TaskValidation
module as proposed in #228Types of changes
What types of changes does your code introduce to FsToolkit.ErrorHandling?
Checklist
Further comments
This will probably warrant a decent amount of scrutiny. The
TaskValidation
module mirrors theAsyncValidation
module, along with its suite of tests, same with the ops. The code for the computation expression and its tests are derived from theTaskResult
computation expression and its tests, since theAsyncValidation
computation expression does not use the state machine. This should probably be fine, sinceValidation
is simply aResult
that accumulates the errors. But it may warrant some additional,Validation
specific tests that I might have overlooked.