-
Notifications
You must be signed in to change notification settings - Fork 720
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
Straight line error handling #4785
Conversation
1ccbace
to
5455aef
Compare
5455aef
to
215c7cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot better than what we had before 👍 . I just want discuss the name of throwNothingM
but happy to approve after.
cardano-api/src/Cardano/Api/Utils.hs
Outdated
@@ -59,6 +61,9 @@ Just x ?! _ = Right x | |||
Left e ?!. f = Left (f e) | |||
Right x ?!. _ = Right x | |||
|
|||
throwNothingM :: Monad m => e -> Maybe a -> ExceptT e m a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on the name throwOnNothingM
? Also why the M?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't think the M
adds anything here. I don't think there's a high risk of someone defining
throwNothing :: e -> Maybe a -> Either e a
throwNothing e = maybe (Left e) Right
but more importantly, I think it should be called something like throwOnNothing
, since it's not throwing the Nothing
. Possibly even flip the order and call it fromMaybeThrowing
to avoid needing an &
.
infix 1 `fromMaybeThrowing`
fromMaybeThrowing :: Monad m => Maybe a -> e -> ExceptT e m a
fromMaybeThrowing Nothing e = throwError e
fromMaybeThrowing (Just a) e = pure a
because I think that might read more nicely
qeInMode <- toEraInMode era CardanoMode
`fromMaybeThrowing` EraConsensusModeMismatch (AnyConsensusMode CardanoMode) (getIsCardanoEraConstraint era $ AnyCardanoEra era)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the &
reads okay. It's a well known operator that I just read it as "and".
For example f & throwOnNothing
I would read as do f
and throw on nothing.
I think keeping the &
is also better because it fits in with other operators like <&>
that we might want to use in other contexts.
Moreover, the foo
works okay for combinators that take two arguments, but in the future, it is conceivable for more combinators to be added that take more or fewer arguments.
I've renamed it to onNothingThrow
because it reads well and the on
prefix is likely to be common to many error handling combinators so having it first so al the on
s align is more aesthetically pleasing when there are multiple handlers on the same expression.
215c7cd
to
f660dbb
Compare
f660dbb
to
247118b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff 👍
(AnyConsensusMode CardanoMode) | ||
(getIsCardanoEraConstraint era $ AnyCardanoEra era) | ||
queryStateForBalancedTx era networkId allTxIns = runExceptT $ do | ||
SocketPath sockPath <- ExceptT $ first SockErr <$> readEnvSocketPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you're not using newExceptT
because ExceptT
is shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's right.
@@ -50,6 +51,7 @@ import System.Directory (emptyPermissions, readable, setPermissions) | |||
#endif | |||
|
|||
import Cardano.Api.Eras | |||
import Control.Monad.Trans.Except (ExceptT, throwE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:'(
The idea with this PR is to demonstrate the use of
ExceptT
to enable a style of code whereby code that is logically a sequence of steps can be expressed as consecutive statements in ado
block.Error handling is achieved by attaching handlers to each statement and recovering or throwing an exception whichever is appropriate for that statement.
This PR does not change the behaviour of the code and only meant to demonstrate how it could be written.
One nice advantaging of coding in this way is that code doesn't need to get re-indented as much. If an extra step is required, it is just inserted as new lines.
With pattern matching, adding a new step could involve indenting code that follows it.
Another advantage of writing code in this style is that the error handling is always close to the code that it handles. In the code before, the error handling is often far away from the code it handles making it more difficult to reconcile the two.
This code was extracted from #4777.