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

Fsdk: added UnwrapEither function #22

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

Conversation

webwarrior-ws
Copy link
Contributor

Added UnwrapEither function.

Added UnwrapEither function.
=
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

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.

@knocte knocte force-pushed the master branch 2 times, most recently from 34818e9 to e2b1c26 Compare March 19, 2023 06:05
@knocte knocte force-pushed the master branch 6 times, most recently from f2a13d9 to b128a51 Compare May 2, 2023 09:00
@knocte knocte force-pushed the master branch 5 times, most recently from c516481 to 9ec0efc Compare June 10, 2023 17:42
@knocte knocte force-pushed the master branch 3 times, most recently from bec6a6d to 87e807e Compare August 22, 2023 07:52
@knocte knocte force-pushed the master branch 2 times, most recently from 598f209 to 7c32dca Compare August 26, 2023 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants