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

Result module parity with Options #1123

Closed
5 tasks done
Happypig375 opened this issue Mar 20, 2022 · 15 comments
Closed
5 tasks done

Result module parity with Options #1123

Happypig375 opened this issue Mar 20, 2022 · 15 comments
Labels

Comments

@Happypig375
Copy link
Contributor

Happypig375 commented Mar 20, 2022

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:

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • 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:

  • This is not a breaking change to the F# language design
  • 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 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@pblasucci
Copy link

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" 😉

@Tarmil
Copy link

Tarmil commented Mar 21, 2022

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.

@theprash
Copy link

@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.

@pblasucci
Copy link

pblasucci commented Mar 21, 2022

@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).

@dsyme
Copy link
Collaborator

dsyme commented Mar 22, 2022

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.

@charlesroddie
Copy link

charlesroddie commented Mar 23, 2022

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

@TheAngryByrd
Copy link

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.

@dsyme dsyme added the good first issue Please consider contributing to this! label Jun 9, 2022
@uxsoft
Copy link

uxsoft commented Jun 11, 2022

I started with the RFC here:
fsharp/fslang-design#667

@cmeeren
Copy link

cmeeren commented Jun 27, 2022

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.

@gusty
Copy link

gusty commented Jul 8, 2022

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.

@TheAngryByrd
Copy link

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.

@cartermp
Copy link
Member

cartermp commented Jul 8, 2022

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

gusty added a commit to gusty/fslang-design that referenced this issue Jul 8, 2022
@gusty
Copy link

gusty commented Jul 8, 2022

Done, thank you both for the quick feedback.

@edgarfgp
Copy link

@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?

@dsyme
Copy link
Collaborator

dsyme commented Oct 27, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests