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

Add TaskValidation module #313

Merged
merged 8 commits into from
Mar 2, 2025
Merged

Conversation

tw0po1nt
Copy link
Contributor

@tw0po1nt tw0po1nt commented Mar 1, 2025

Proposed Changes

Add TaskValidation module as proposed in #228

Types of changes

What types of changes does your code introduce to FsToolkit.ErrorHandling?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Build and tests pass locally
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Further comments

This will probably warrant a decent amount of scrutiny. The TaskValidation module mirrors the AsyncValidation module, along with its suite of tests, same with the ops. The code for the computation expression and its tests are derived from the TaskResult computation expression and its tests, since the AsyncValidation computation expression does not use the state machine. This should probably be fine, since Validation is simply a Result that accumulates the errors. But it may warrant some additional, Validation specific tests that I might have overlooked.

Copy link
Contributor Author

@tw0po1nt tw0po1nt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-review complete

Copy link
Collaborator

@TheAngryByrd TheAngryByrd left a 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"
Copy link
Collaborator

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

is working.

Copy link
Contributor Author

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 =
Copy link
Collaborator

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added these overloads

Copy link
Collaborator

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ Thank you!

@TheAngryByrd TheAngryByrd merged commit 93804f4 into demystifyfp:master Mar 2, 2025
27 checks passed
TheAngryByrd added a commit that referenced this pull request Mar 2, 2025
- 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
TheAngryByrd added a commit that referenced this pull request Mar 5, 2025
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants