Add IdentityT #121
Add IdentityT #121
Conversation
|
Build is failing with this error. I think this was explained on Discourse somewhere. $ TAG=$(wget -q -O - https://github.com/purescript/purescript/releases/latest --server-response --max-redirect 0 2>&1 | sed -n -e 's/.*Location:.*tag///p') 0.19s$ wget -O $HOME/purescript.tar.gz https://github.com/purescript/purescript/releases/download/$TAG/linux64.tar.gz --2020-04-08 00:17:14-- https://github.com/purescript/purescript/releases/download//linux64.tar.gz Resolving github.com (github.com)... 140.82.114.3 Connecting to github.com (github.com)|140.82.114.3|:443... connected. HTTP request sent, awaiting response... 404 Not Found 2020-04-08 00:17:14 ERROR 404: Not Found. The command "wget -O $HOME/purescript.tar.gz https://github.com/purescript/purescript/releases/download/$TAG/linux64.tar.gz" failed and exited with 8 during . |
|
Ah yeah, here's the relevant thread: https://discourse.purescript.org/t/new-404-ci-failures-in-core-libraries/1225/8?u=hdgarrood Would you like to try updating this repo? I think preferably the Travis fix would go into a separate PR. |
|
Mind retriggering this build now that the CI script has been fixed? |
|
Just restarting the build didn't seem to work, will try closing and reopening this. If that doesn't work, might need to rebase/merge master. |
| instance applicativeIdentityT :: Applicative m => Applicative (IdentityT m) where | ||
| pure a = IdentityT (pure a) | ||
|
|
||
| instance altIdentityT :: (Monad m, Alt m) => Alt (IdentityT m) where |
There was a problem hiding this comment.
Do we need the Monad constraint here?
There was a problem hiding this comment.
Alt only requires Functor, so perhaps this instance could be (Functor m, Alt m) => Alt (IdentityT m)?
There was a problem hiding this comment.
Probably, I mainly copied/pasted from the Haskell library.
There was a problem hiding this comment.
Fixed in latest commits
|
|
||
| instance monadErrorIdentityT :: MonadError e m => MonadError e (IdentityT m) where | ||
| catchError (IdentityT m) h = | ||
| IdentityT $ catchError m (\a -> case h a of IdentityT b -> b) |
There was a problem hiding this comment.
I would probably write this as IdentityT $ catchError m (runIdentityT <<< h)
There was a problem hiding this comment.
Fixed in latest commits
| instance monadIdentityT :: Monad m => Monad (IdentityT m) | ||
|
|
||
| instance monadRecIdentityT :: MonadRec m => MonadRec (IdentityT m) where | ||
| tailRecM f a = IdentityT $ |
There was a problem hiding this comment.
This instance looks a bit suspicious to me. I think it might not be stack-safe. Is it using the MonadRec m constraint? I.e. does the code still compile if you remove the constraint?
There was a problem hiding this comment.
Yeah, removing that would make it compile as long as it had a Monad constraint on the m. I believe the implementation in the latest commits properly implement it now.
There was a problem hiding this comment.
Yes, the current implementation looks great 👍
| instance altIdentityT :: (Functor m, Alt m) => Alt (IdentityT m) where | ||
| alt (IdentityT x) (IdentityT y) = IdentityT (x <|> y) | ||
|
|
||
| instance plusIdentityT :: (Monad m, Plus m) => Plus (IdentityT m) where |
There was a problem hiding this comment.
I think this one only needs Functor m too?
There was a problem hiding this comment.
Yeah. Fixed.
There was a problem hiding this comment.
Does it really need Functor? For context, see this image of our type class hierarchy. Seems like Plus should be enough:
https://github.com/JordanMartinez/purescript-jordans-reference/blob/latestRelease/41-Ecosystem/Type-Classes/Type-Class-Relationships.svg
There was a problem hiding this comment.
Yes, good point! since Plus m already implies Alt m, which implies Functor m.
| instance plusIdentityT :: (Monad m, Plus m) => Plus (IdentityT m) where | ||
| empty = IdentityT empty | ||
|
|
||
| instance alternativeIdentityT :: (Monad m, Alternative m) => Alternative (IdentityT m) |
There was a problem hiding this comment.
I think we only need Applicative m here
There was a problem hiding this comment.
Oh but also Applicative is implied by Alternative, so I think we can probably just get rid of the Monad constraint entirely.
There was a problem hiding this comment.
Yup. You're right. This has been fixed in latest commits.
🤦♂️ You're right! Wow... I can't believe I did that. It's not the first time either... |
Fixes #120
I likely haven't added every instance that
IdentityTcould have, and I'm not sure whether all of these instances are law-abiding. What I have below was based onMaybeT. I'm guessing this should also have instances for the Contravariant transformer classes as well?