fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

Result module parity with Options

Open Happypig375 opened this issue 3 years ago • 14 comments

Result module parity with Options

I propose we add

get
isOk
isError
defaultValue
defaultWith
count
fold
foldBack
exists
forall
contains
iter
toArray
toList
toSeq
toOption
toValueOption

to the Result module to have parity with Option and ValueOption modules.

The existing way of approaching this problem in F# is falling back to match expressions.

Pros and Cons

The advantages of making this adjustment to F# are

  1. Convenience
  2. Conciseness
  3. Consistency

The disadvantages of making this adjustment to F# are more functions needed to learn.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions:

  • no orElse/orElseWith/(flatten/map2/map3) because combining Errors are ambiguous: https://github.com/fsharp/fslang-suggestions/issues/650

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [x] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [x] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [x] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [x] This is not a breaking change to the F# language design
  • [x] I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

Happypig375 avatar Mar 20 '22 18:03 Happypig375

I'm generally a fan of this, with one exception...

Instead of add Result.get can we -- pretty please -- deprecate Option.get?

I'd even be happy with both modules having a function named .getOrRaise. We could file this under "Making F# simpler to learn" :wink:

pblasucci avatar Mar 21 '22 13:03 pblasucci

Also, do we want a version of some or most of these that works on the error instead of the result? Considering that there is already a mapError.

Tarmil avatar Mar 21 '22 13:03 Tarmil

@pblasucci Maybe off topic, but I think it would be even more valuable to deprecate option's .Value property because that's way too easy to use.

theprash avatar Mar 21 '22 17:03 theprash

@theprash Oh, for sure. I'd like to see them both go the way of the dodo. And, in truth, I'd expect changes to the surface of 'T option and the Option module to be in a separate issue (together).

pblasucci avatar Mar 21 '22 17:03 pblasucci

The original suggestion looks entirely reasonable, I'll mark it as approved. I agree with omitting Result.get. We can consider Option.get in a separate suggestion

Also, do we want a version of some or most of these that works on the error instead of the result? Considering that there is already a mapError.

I think not.

dsyme avatar Mar 22 '22 00:03 dsyme

Good

isOk
isError
defaultValue
defaultWith
toOption
toValueOption

Seq-like, unclear why only Oks chosen

These assume that Result<'T,'U> is essentially an 'IEnumerable<'T>'. It is not clear why it is the OK values that are enumerated and not the Error ones.

Solutions:

  1. Append Ok to all these names
  2. Don't do anything and require the user to do r.ToOption |> Option.iter or r.ToSeq etc..
  3. Have Result<'T,'U> implement IEnumerable<'T>. This doesn't deal with the problem but removes the need to have these methods and the associated inconsistency of doing seq-like things but not being a seq.
count
forall
toSeq
contains
fold
exists
iter
toArray
toList

Bad

get
foldBack

charlesroddie avatar Mar 23 '22 21:03 charlesroddie

When adding this, I would want us to consider adding InlineIfLambda attribute to appropriate parameters. This discussion suggests putting it in Option/ValueOption and I'd assume we'd want the same for Result for the same reasons. I implemented this recently in FsToolkit.ErrorHandling and the performance/SharpLap really speak to why I think it would be worth doing in FSharp.Core.

TheAngryByrd avatar Apr 04 '22 13:04 TheAngryByrd

I started with the RFC here: https://github.com/fsharp/fslang-design/pull/667

uxsoft avatar Jun 11 '22 21:06 uxsoft

I'm generally a fan of this, with one exception...

Instead of add Result.get can we -- pretty please -- deprecate Option.get?

I'd even be happy with both modules having a function named .getOrRaise. We could file this under "Making F# simpler to learn" 😉

I'd actually like Result.get. Even as a fairly advanced F# programmer, I use Option.get surprisingly often. Mostly when I do a DB lookup of a single record (the DB lookup itself of course returns an option since you're never statically guaranteed to get any results), and in the context that lookup happens, I know for a fact that it will always exist and are happy to get a nondescript exception (the stack trace is enough) if my assumption was incorrect.

I could live with getOrRaise, but I prefer get. personally I see little reason to deprecate get and replace it by an identical getOrRaise, though if anyone knows for a fact this is a common pain point for F# beginners, by all means do it.

cmeeren avatar Jun 27 '22 19:06 cmeeren

I just read the RFC, for the function: val defaultWith: defThunk: (unit -> 'T) -> result: Result<'T, 'Error> -> 'T I propose to adjust it to match what we did at F#+ which is basically the same but the defThunk is ('Error -> 'T) instead of (unit -> 'T) which makes more sense and give us the opportunity to use the error value in the compensation.

gusty avatar Jul 08 '22 16:07 gusty

I just read the RFC, for the function: val defaultWith: defThunk: (unit -> 'T) -> result: Result<'T, 'Error> -> 'T I propose to adjust it to match what we did at F#+ which is basically the same but the defThunk is ('Error -> 'T) instead of (unit -> 'T) which makes more sense and give us the opportunity to use the error value in order to compensate for errors.

Agreed, FsToolkit should have done this and I regret not doing it sooner. It’s easy to throw away the error if it doesn’t matter.

TheAngryByrd avatar Jul 08 '22 16:07 TheAngryByrd

@gusty would you like to make a PR to the RFC? I generally agree

cartermp avatar Jul 08 '22 16:07 cartermp

Done, thank you both for the quick feedback.

gusty avatar Jul 08 '22 18:07 gusty

@vzarytovskii I think this can be closed now that has been implemented :). Might be worth adding Option, ValueOption, Result to be eligible to use [<InlineIfLambda>] as part of as https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1115-InlineIfLambda-in-FSharp-Core.md as @TheAngryByrd suggested?

edgarfgp avatar Jul 31 '22 18:07 edgarfgp

RFC: https://github.com/fsharp/fslang-design/blob/main/FSharp-7.0/FS-1123-result-module-parity-with-option.md

Implementation: https://github.com/dotnet/fsharp/pull/13326

dsyme avatar Oct 27 '22 13:10 dsyme