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.

Sign up for GitHub

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

Jump to bottom

Fsdk: added UnwrapEither function #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fsdk: added UnwrapEither function #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Fsdk/FSharpUtil.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,13 @@ module FSharpUtil =
ReRaise ex |> ignore<Exception>
failwith "unreachable"

let UnwrapEither<'T, 'TExn when 'TExn :> Exception>
(eitherValue: Either<'T, 'TExn>)
=
match eitherValue with
| SuccessfulValue result -> result
| FailureResult ex -> raise <| ReRaise ex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webwarrior-ws FSharpUtil.ReRaise is for async jobs, not this; read the comment that is placed in ReRaise function please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webwarrior-ws FSharpUtil.ReRaise is for async jobs, not this; read the comment that is placed in ReRaise function please

That is what NOnion's UnwrapResult was using.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aarani before we use this in NOnion let's fix this ^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aarani ping

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused you want to make a commit to fix something you want to replace afterward?

Copy link
Contributor

@aarani aarani May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webwarrior-ws @knocte should correct me but I guess raise ex is enough here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realised now what's wrong here. Look, you're mixing error-handling models. There's one model: exceptions, and then there's another model: Result types. With this thing you're mixing both models in one: a Failure monad that has an exception type??? This is super weird. Resullt types should contain error types which are normally DUs, not Exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's a questionable design choice. But it's used all over NOnion.
So what do we do with OperationResult and UnwrapResult?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's used all over NOnion.

Let's work on fixing this first then. Taras can do it to offload you from it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's work on fixing this first then. Taras can do it to offload you from it.

Oops I thought I was replying to Afshin. Anyway, my comment still stands.


type OptionBuilder() =
// see https://github.com/dsyme/fsharp-presentations/blob/master/design-notes/ces-compared.md#overview-of-f-computation-expressions
member x.Bind(v, f) =
Expand Down