ExtCore icon indicating copy to clipboard operation
ExtCore copied to clipboard

Supporting Result and/or Choice

Open jack-pappas opened this issue 6 years ago • 3 comments

For ExtCore v1.0 -- should we support both the Result<_,_> and Choice<_,_> type for error-handling, or should we drop Choice<_,_> and use only Result<_,_>?

Pros/cons of supporting Result<_,_> only:

  • :+1: Smaller code size, less maintenance work (long-term)
  • :+1: Just one way of doing things -- maybe simpler for less-experienced F# devs
  • :-1: Result<_,_> is a struct, so doing a blanket replacement throughout code may hurt performance in cases where a Result is created wrapping a large struct and thereby induces expensive struct copies.
  • :-1: Eliminating support for Choice<_,_> breaks backwards-compatibility with code written against current/past versions of ExtCore. Users of the library will have to to make non-trivial changes to their code in order to upgrade to ExtCore v1.0.

Pros/cons of supporting Result<_,_> and Choice<_,_>:

  • :+1: Provides backwards-compatibility for code already written against current/past versions of ExtCore. Users should be able to (with the exception of the breaking changes defined in #47) simply upgrade to ExtCore v1.0 when it's released.
  • :+1: Users can still choose between Result<_,_> and Choice<_,_> and use whatever best fits their needs.
  • :-1: Adds additional complexity to ExtCore -- wherever we support error-handling within functions, we now need to implement those functions twice (once for Result, once for Choice). This is transitive as well -- any functions that call these functions also need to be implemented twice, etc.

cc @vasily-kirichenko @7sharp9 @wallymathieu @dsyme @tihan

jack-pappas avatar Jul 15 '18 19:07 jack-pappas

I've seen some non trivial code that still uses Choice, for instance Suave.

Question is how much of your own effort you want to spend (to support backwards compatibility)?

wallymathieu avatar Jul 16 '18 11:07 wallymathieu

I think that perhaps @vasily-kirichenko has the most experience using a dual Result<,> Choice<,> ExtCore. Though I remember that there was a lot of code (in that PR) in order to support that solution. For f#+ @gusty opted to start out with changing Choice<,> to Result<,> and then re-add Choice<,> based error handling with new name to support interop with libraries that still use Choice<,>.

Perhaps a compatibility method Result.OfChoice and a smaller set of combined builders could be used to limit the need for support?

wallymathieu avatar Jul 16 '18 11:07 wallymathieu

@wallymathieu yes, I just pulled your PR, built a nuget package and have used it (Choice and Result are both there) on production with zero problems. So, I'm for just merge that PR.

vasily-kirichenko avatar Jul 16 '18 16:07 vasily-kirichenko