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 asyncValidation CE, or make asyncResult (and result?) CEs work with 'and!' #209

Closed
cmeeren opened this issue Jan 31, 2023 · 4 comments
Closed
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Jan 31, 2023

I just discovered that I can't use and! in result { } and expect my errors to accumulate; I have to use the validation { }. However, I can't find a corresponding async variant. I have an asyncResult computation expression that uses and!, and I want the errors to accumulate. Is that possible, and if not, could you consider implementing it?

If it's at all possible, I think the best course of action would be to accumulate errors when using and! if the error type is 'a list. This includes result { } and other similar CEs. Currently, result { } supports and! (i.e., the code compiles and runs), but there seems to be no difference between and! and successive let!, so it seems rather pointless. If result { } should not support applicative error accumulation, it would be better if and! was not supported at all; that would make it clear to users that they can't rely on applicative behavior, unlike now, where that is easily assumed but is not the case.

@TheAngryByrd TheAngryByrd added enhancement New feature or request good first issue Good for newcomers labels Jan 31, 2023
@TheAngryByrd
Copy link
Collaborator

Yeah I considered adding an asyncValidation in the past but it wasn’t something I needed.

I’m not sure how adding it to result would work as it would making dealing with list and non list would be difficult. If there’s no real performance benefits of the and! in result then it makes sense to remove it. Additionally MergeSouces to asyncResult means does async actually run in parallel here too? Don thinks it should be its own CE for various reasons.

I’d accept a PR for an asyncValidation/taskValidation/jobValidation.

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 31, 2023

Thanks for approving this. Unfortunately I'm swamped both at work and at home right now, so I have no capacity to create a PR for this (which would require me to understand the internals of the validation and asyncResult CEs and "merge" them accordingly, write/adapt lots of tests, etc.)

@Swoorup
Copy link
Contributor

Swoorup commented Jun 15, 2023

taskValidation is quite useful and encompasses everything, http/db calls requests, etc.

If anyone wants to take upon this, you can use this as a starting point. This fulfills my current needs but I don't have the capacity to expand this further atm.

https://gist.github.com/Swoorup/ae556dddf4926faa3209a7ffaaa45cc4

@TheAngryByrd
Copy link
Collaborator

Removed MergeSources (so you can no longer and!) from:

  • Result
  • ResultOption
  • AsyncResult
  • Option
  • ValueOption

I did not remove them from:

  • TaskResult
  • TaskOption

as those have some parallelism that still can be useful.

Will be available in v5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants