Make FsToolkit.ErrorHandling a superset of Cvdm.ErrorHandling?

[cmeeren]

This library looks great! Good documentation and more comprehensive than Cvdm.ErrorHandling with regards to operators and choice of CEs (asyncResultOption).

Currently Cvdm.ErrorHandling has certain important features I don't immediately see in FsToolkit.ErrorHandling (having read the docs, not tested or browsed the code to any significant degree). I think I'd like for FsToolkit.ErrorHandling to be a superset of Cvdm.ErrorHandling and then simply retire the latter. The goals seem similar enough that I think this could be feasible, and it would benefit users by removing an unnecessary choice. How do you feel about that? Specifically:

• Cvdm.ErrorHandling contains overloads on the asyncResult CE builder so that it can bind/return Result<_,_> expressions, not just Async<Result<_,_>>. This is a must for me as I use Result<_,_> and Async<Result<_,_>> expressions interchangeably all the time (in the same CEs), and I won't move to another error handling library that doesn't have this. Not sure how the overload trick will work with AsyncResultOption though, since it has three wrappers instead of two. Might work fine, might not work. Still, supporting this in AsyncResult is better than none.

• Cvdm.ErrorHandling contains more members on the CE builders. Not sure from the top of my head what the practical differences are, but running relevant unit tests in Cvdm.ErrorHandling against FsToolkit.ErrorHandling should surface any missing features and incongruities in behaviour.

• Cvdm.ErrorHandling contains quite a few helpers in the Result and AsyncResult modules (e.g. requireSome, requireTrue, requireEqual) to make it syntactically simpler to do ad-hoc validation in CEs when separate functions aren't really needed. See the readme for a couple of examples.

• Cvdm.ErrorHandling is AutoOpened. For the rationale behind this, see Set AutoOpen attribute for the assembly cmeeren/Cvdm.ErrorHandling#1.

Thoughts?

[demystifyfp]

Hi @cmeeren,

Thanks for taking your time to put the questions together.

To give background behind this library, I was a long time user of Chessie. In all the projects where I used Chessie, I kept carrying some utility functions around. After the introduction of the Result type (in F#) & Cvdm.ErrorHandling I found myself repeating the same. Hence I created the library with all those functions & CE that I was using in my projects. I never thought of the making this as a superset (or replacement of Cvdm) as I intended to share what I had.

I second your thoughts on making as a superset, and I am open to merging the two projects and providing a unified library.

Cvdm.ErrorHandling contains overloads on the asyncResult CE builder so that it can bind/return Result<,> expressions, not just Async<Result<,>>. This is a must for me as I use Result<,> and Async<Result<,>> expressions interchangeably all the time (in the same CEs), and I won't move to another error handling library that doesn't have this

We can have this overload here as well.

Cvdm.ErrorHandling contains more members on the CE builders. Not sure from the top of my head what the practical differences are, but running relevant unit tests in Cvdm.ErrorHandling against FsToolkit.ErrorHandling should surface any missing features and incongruities in behaviour.

My answer to the previous one applies here as well.

The current functionalities represent what I used/required in my projects. We can add the others as well to make it useful for the overall community.

I can add them in the next release, and I am open to your PR as well if you want to contribute.

Cvdm.ErrorHandling contains quite a few helpers in the Result and AsyncResult modules (e.g. requireSome, requireTrue, requireEqual) to make it syntactically simpler to do ad-hoc validation in CEs when separate functions aren't really needed. See the readme for a couple of examples.

For the above requirement, I am thinking to provide a solution similar to this https://vaskir.blogspot.com/2015/05/composing-custom-error-types-in-f.html. Currently, I don't have a concrete idea on how to go about it, and I am open to your suggestion.

Cvdm.ErrorHandling is AutoOpened. For the rationale behind this, see cmeeren/Cvdm.ErrorHandling#1.

I have mixed experiences with AutoOpen, on the good side it avoids the need of an explicit open statement but on the bad side it may be a noise to the user, If they didn't need it in a given file. Hence, I am not sure on how to go about it. References: (https://twitter.com/sforkmann/status/712224238317510656, https://twitter.com/sforkmann/status/869088926824321024)

[cmeeren]

Glad to hear you're positive!

For the above requirement, I am thinking to provide a solution similar to this https://vaskir.blogspot.com/2015/05/composing-custom-error-types-in-f.html. Currently, I don't have a concrete idea on how to go about it, and I am open to your suggestion.

I think perhaps we misunderstand each other here, because I did not see anything in that blog post that was relevant to my point. I am talking about the functions defined in Helpers.fs. They assist in various stuff such as inline validation by converting simple values to Result (requireTrue, requireFalse, requireEqual, requireEmpty, requireNotEmpty), converting option to Result (requireSome, requireNone), extracting values without needing to explicitly pattern match for simple usecases (requireHead, requireSome as mentioned), many standard functions similar to what is in the Option module (defaultValue, defaultWith, etc.) as well as some custom ones I've found useful (withError, setError, tee, teeIf, teeError, teeErrorIf). I've personally found most of these functions very helpful in expressing validation in a short and concise manner when using both in and out of the CEs.

As for implementing them in FsToolkit.ErrorHandling, the functions as well as their tests can of course be copied directly from Cvdm.ErrorHandling.

Please take a look at Helpers.fs for details - do you agree that these would be helpful in FsToolkit.ErrorHandling?

---

I have mixed experiences with AutoOpen, on the good side it avoids the need of an explicit open statement but on the bad side it may be a noise to the user, If they didn't need it in a given file.

I agree that it must be used very carefully to avoid unnecessary noise. In all my uses of Cvdm.ErrorHandling however, I find that I often use the CEs and the helper functions at most levels of my app, from basic domain logic to the application layer (e.g. Giraffe HttpHandlers). I would likely need to add open statements to at least 80% of my files if I had not used AutoOpen.

The imported names in the case of Cvdm.ErrorHandling are the CEs (result and asyncResult, just two names) as well as the helper functions in the Result and AsyncResult modules (only visible when dotting into Result, where they are as relevant as any List function is when using lists). I have this have never found AutoOpen to incur unnecessary noise for Cvdm.ErrorHandling, and no users have complained so far. :) In any case, AutoOpen isn't a requirement for me. I find it useful for this kind of library, but if you don't want it, I can live without it.

---

In general: I have no problem letting you handle these changes yourself if you prefer that (much can be copied directly from Cvdm.ErrorHandling, and modified to suit your desired style). I am generally busy (who isn't?), particularly this weekend and the coming week. But if you want, I can help you with PRs (without promises on any time-frame). Do let me know if you want me to contribute anything specific; otherwise, I'll let you handle it.

[demystifyfp]

Sorry, @cmeeren for the delayed response. I was held up with some official & personal work.

I went through the Helpers.fs. It makes sense, and I will add them in the next releases.

Regarding, AutoOpen, can we wait for some time & a few more feedback before making a call?

I will create issues in GitHub for all missing features, whenever I start working on them (most likely coming weekend or next week) so that you will get an idea on what I am working on. Feel free to jump in create an issue/submit a pull request (with WIP status), if would like to collaborate.

[cmeeren]

I went through the Helpers.fs. It makes sense, and I will add them in the next releases.

Great!

Regarding, AutoOpen, can we wait for some time & a few more feedback before making a call?

Absolutely.

I will create issues in GitHub for all missing features, whenever I start working on them (most likely coming weekend or next week) so that you will get an idea on what I am working on. Feel free to jump in create an issue/submit a pull request (with WIP status), if would like to collaborate.

Noted. I'll watch this repo to stay updated.

As a final note, please do run the Cvdm.ErrorHandling unit tests against FsToolkit.ErrorHandling to see if there are any differences in behavior, particularly regarding the CE members. I'm no expert in CEs and have had some surprises along the way, so I basically wrote as complete a set of unit tests I could think of both to verify what's supported (e.g.for, if), as well as how the code is executed (e.g. that both branches of if ... else are not excecuted, which I think happens if you don't have Delay and Run).

[cmeeren]

Just want to let you know that there was a lot more to CEs than I had expected, and some of my original decisions turned out incorrect. I've released v2.0.0 of Cvdm.ErrorHandling now, with rewritten CEs and added tests. I have also added overloads accepting Task<_> and Task expressions to make interop easier.

[demystifyfp]

Thanks for the information. I am held up with my office and personal commitments. Will incorporate your changes when I start working on this.

…

On Sat, 10 Nov 2018, 00:42 Christer van der Meeren ***@***.*** wrote: Just want to let you know that there was a lot more to CEs than I had expected, and some of my original decisions turned out incorrect. I've released v2.0.0 of Cvdm.ErrorHandling now, with rewritten CEs and added tests. I have also added overloads accepting Task<_> and Task expressions to make interop easier. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AdgLEvRzIEyyIHguM4SrB-uDLFgvRCp_ks5utdOZgaJpZM4XaP0U> .

[AlbertoDePena]

Not sure the status of this issue but I also find that some things should be included. I have a gist that I always include in my FSharp projects (https://gist.github.com/AlbertoDePena/c91111a675d9c513cd09857efbd29aa7). Would it be possible to include some of these functions? Most, if not all, of it was a direct port from a talk I saw by Scot Wlaschin.

[cmeeren]

@AlbertoDePena I haven't looked too closely at them, but many of them are already part of Cvdm.ErrorHandling under different (more semantic, less railway-analogous) names. See Helpers.fs.

[cmeeren]

@demystifyfp By the way, regarding the (identical) requireEqualTo and requireEqual functions, I've thought that it would probably be better to just unify them under the name requireEquals, which sounds good with both partial application (pipelining) and full application. Feel free to do that when incorporating them.

[demystifyfp]

Hi @AlbertoDePena Thanks for your comment. Currently, I am held up and I am planning to act on this in the last week of Jan. Will consider your gist while unifying both the libraries.

@cmeeren Sure.

[AlbertoDePena]

@demystifyfp just a thought...
Would it make sense to include the map, bind, return and apply functions for Option, Async, Result, List? I realize most if not all of these are natively provided by FSharp.Core but I am saying only those that are missing...

BTW, this library is pure gold! Fantastic job...

[cmeeren]

Would it make sense to include the map, bind, return and apply functions for Option, Async, Result, List? I realize most if not all of these are natively provided by FSharp.Core but I am saying only those that are missing...

My two cents: If they are included in FSharp.Core, there is no reason to include them here. And even those that are not included should only be included here if they relate to error handling in some way, to keep the library focused.

[demystifyfp]

Hi @AlbertoDePena,

Thanks for the compliment. All credits to the wonderful F# community!

I second @cmeeren here. The objective of this library is making error handling in F# easier. If you feel certain functions need to be added here, feel free to create an open a Work In Progress PR or an issue with your thoughts.

[demystifyfp]

@cmeeren Finally got some time to push the changes. I started with porting the helper functions for Result. Except for the following two changes, ported everything.

• Dropped withError as setError already covers this scenario. Refer the test case
• Dropped requireEqualTo & requireEqual in favor of requireEquals.

The next plan is to port the helper functions for AsyncResult and finally the CE functions. Hopefully, by end of this month, we'll be able to complete this migration.

Released a new NuGet version too!

[cmeeren]

Great!

I strongly recommend taking withError, too. As the function's documentation states, it's a safer alternative to setError since it requires the error case to be unit, so you're not losing any information by setting the specific error value. setError replaces whatever error information was there, so in general you're losing information.

I use withError all the time in functions that return Result<'a,unit> (i.e., that currently only has one general error condition) because they might later change to Result<'a,'b> (i.e, they might be expanded with new error cases), because with setError I'd get no compiler warning that I'm suddenly losing information going from unit to 'b. It's in principle the same as having a catch-all _ in a pattern matching expression – you lose the valuable completeness check.

[demystifyfp]

Thanks @cmeeren

I use withError all the time in functions that return Result<'a,unit> (i.e., that currently only has one general error condition) because they might later change to Result<'a,'b> (i.e, they might be expanded with new error cases), because with setError I'd get no compiler warning that I'm suddenly losing information going from unit to 'b. It's in principle the same as having a catch-all _ in a pattern matching expression – you lose the valuable completeness check.

I like this rationale. As I never faced this in my F# projects, I couldn't see the difference.

Will add it in the next update.

[demystifyfp]

Hi @cmeeren, All the helper functions from Cvdm.ErrorHandling is now part of FsToolkit.ErrorHandling version 0.0.14.

Will be working on CE next!

[cmeeren]

It seems I was mistaken regarding dropping requireEqualTo. I was just thinking of the name – requireEquals fits just as well using both normal and partial application (piping). But of course it's not just the name that's different, it's the argument order too. I had entirely forgotten that when I recommended dropping it.

I suggest including requireEqualTo, because the argument order is much more suitable for using at the end of a pipe chain. For example:

do! foo |> doA |> doB |> Result.requireEqualTo bar SomeErr

This does not work with requireEquals, because it takes the error as the third argument. One would have to do

do! foo |> doA |> doB |> (fun intermediate -> Result.requireEquals intermediate bar SomeErr)

which, I am sure you agree, is much less clear.

Personally I would then also prefer requireEquals to be named requireEqual, both because it sounds slightly better in my mind when using normal function application, and because it's one less breaking change when existing users migrate from Cvdm.ErrorHandling to FsToolkit.ErrorHandling.

[demystifyfp]

How about adding requireEqualTo & requireEqual in the next release and mark requireEquals as deprecated in favor of ``requireEqual` ?

[cmeeren]

Sounds good, thanks 👍

[demystifyfp]

@cmeeren @TheAngryByrd Thanks for your work on making it possible. All your code changes are pushed as part of the new release (first major release) https://www.nuget.org/packages/FsToolkit.ErrorHandling/1.0.0
Can you folks do high-level testing to make sure that we haven't missed anything obvious? Thanks in advance.

[cmeeren]

All Cvdm.ErrorHandling tests pass using FsToolkit.ErrorHandling 1.0.0. 👍

I recommend copying the extensive tests from Cvdm.ErrorHandling too, to prevent regressions in case the CE code is ever modified.

[demystifyfp]

Thanks, @cmeeren. Let me capture it as a separate issue(#16) and close this one.