UX around Lift #161

googleson78 · 2019-07-05T20:44:58Z

Closes #142

Not sure if I should:

Commit the new .cabal
Bump package version
Add a "since" haddock to runLift

src/Polysemy/Lift/Type.hs

isovector

Looks great, thanks for tackling this. Left some comments inline. To answer your questions:

I don't really care about the cabal file or not. It'll get fixed via merging one way or another.
Please don't bump the package version
Add a -- TODO(sandy): @since to the haddocks instead, which I search for before releasing new versions.

src/Polysemy/Lift.hs

isovector · 2019-07-05T21:11:40Z

src/Polysemy/Lift.hs

+    Lift (..)
+
+    -- * Actions
+  , lift


Traditionally this has been called sendM but admittedly that is a crap name. I'm a little concerned about the overlap with MonadTrans.trans --- but I have to say this is nice. Thoughts?

If we go this way, let's remove sendM across the codebase and reexport lift from Polysemy.

Ah, I just did this because I was following convention (and was sleepy).

I like lift more, because there is no non-Internal send for sendM to be "based off". (like map -> mapM).

It has the same idea as MonadTrans.lift so I don't think that's an issue. (?)

I went ahead and did the rename, if you want to use it.

Thanks for the rename! Upon looking at the changes though, I'm already confused by the name. I think overloading lift here is only going to lead to confusion in the library. Can we come up with some other name (possibly also for the type itself?)

I don't have any particularly bright ideas:

Name(d) - because you're giving a "name" (syntax) to a thing that already has an interpretation (inspired by logic, where you do the same when you want to refer to an element of your universe by creating a fresh constant)

Concrete/Unambiguous (or some nice sounding synonym) - because you're not leaving anything "open to interpretation" (in the Syntax vs Semantics sense in which the other "native" effects are open to "interpretation")

src/Polysemy/Lift.hs

isovector · 2019-07-07T15:32:25Z

How about we name the effect `Embed`, with action `embed`?

…

On Sun, Jul 7, 2019 at 3:42 AM Georgi Lyubenov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Polysemy/Lift.hs <#161 (comment)> : > @@ -0,0 +1,27 @@ +{-# LANGUAGE TemplateHaskell #-} + +module Polysemy.Lift + ( -- * Effect + Lift (..) + + -- * Actions + , lift I don't have any particularly bright ideas: - Name - because you're giving a "name" (syntax) to a thing that already has an interpretation (inspired by logic, where you do the same when you want to refer to an element of your universe with a constant) - Concrete/Unambiguous (or some nice sounding synonym) - because you're not leaving anything "open to interpretation" (in the Syntax vs Semantics sense in which the other "native" effects are open to "interpretation") — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#161?email_source=notifications&email_token=AACLAF5QKN5GGOGE7RD2V6LP6GM7DA5CNFSM4H6N2IU2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5VCDRY#discussion_r300853910>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACLAFYHFQ6GAT7WHX7MHLLP6GM7DANCNFSM4H6N2IUQ> .

googleson78 · 2019-07-07T19:41:44Z

src/Polysemy.hs

-  , Lift (..)
-  , sendM
+  , Embed (..)
+  , embed

    -- * Lifting


Perhaps this also need a better section name (?)

I think it's ok! But keen eye!

googleson78 · 2019-07-07T19:42:19Z

Renamed to Embed.

googleson78 · 2019-07-07T21:03:46Z

src/Polysemy/Internal.hs

-{-# INLINE sendM #-}
+-- | Embed a monadic action @m@ in 'Sem'.
+--
+-- TODO(sandy): @since


Not sure if this actually needs a @since, added just in case.

This thing has been around since v0, but I guess it doesn't hurt to be explicit about that! Good catch!

I was thinking more does it needs a @since for whatever version this will be in, because it's renamed.

Yup, you're right! That's what I get for reviewing just before bed.

isovector

This is excellent. A few last renamings and then good to go! Thanks again for this!

isovector · 2019-07-07T22:46:01Z

src/Polysemy.hs

-  , Lift (..)
-  , sendM
+  , Embed (..)
+  , embed

    -- * Lifting


I think it's ok! But keen eye!

isovector · 2019-07-07T22:46:52Z

src/Polysemy/Embed.hs

+-- run a @Embed m1@ effect by transforming it into a @Embed m2@ effect.
+--
+-- TODO(sandy): @since
+runEmbed


Let's go with runEmbedded

Is this the same thing that exists in Polysemy.IO? This is admittedly a better place for it.

This is the original runLift (now renamed to runEmbed, because the effect is Embed) from #142, which started this pr.

It's not the same thing as Polysemy.IO.runEmbedded.

Personally I'm fine with it being runEmbed (in accordance with the other effects), but we need to rename the second one if this is going to be runEmbedded.

isovector · 2019-07-07T22:49:08Z

src/Polysemy/IO.hs

 {-# INLINE runIO #-}


 ------------------------------------------------------------------------------
-- | Given some @'MonadIO' m@, interpret all @'Lift' m@ actions in that monad
+-- | Given some @'MonadIO' m@, interpret all @'Embed' m@ actions in that monad


Let's rename this to runEmbeddedInIO

Ah, I didn't see this before commenting above.

isovector · 2019-07-07T22:49:51Z

src/Polysemy/Internal.hs

-{-# INLINE sendM #-}
+-- | Embed a monadic action @m@ in 'Sem'.
+--
+-- TODO(sandy): @since


This thing has been around since v0, but I guess it doesn't hurt to be explicit about that! Good catch!

* runEmbedded -> runEmbeddedInIO * runEmbed -> runEmbedded

googleson78 · 2019-07-11T06:51:10Z

Is this ready to go?

isovector · 2019-07-11T14:40:55Z

Thanks for the reminder, it slipped off my radar. Let's get it merged today!

…

On Thu, Jul 11, 2019 at 2:51 AM Georgi Lyubenov ***@***.***> wrote: Is this ready to go? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#161?email_source=notifications&email_token=AACLAF5BGSJADCFZHJJFHSTP63J55A5CNFSM4H6N2IU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZVWG2A#issuecomment-510354280>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACLAFZ7DIG2FQYNAGSSAFDP63J55ANCNFSM4H6N2IUQ> .

isovector · 2019-07-11T15:02:35Z

Excellent work. Thank you so much!

googleson78 commented Jul 5, 2019

View reviewed changes

src/Polysemy/Lift/Type.hs Outdated Show resolved Hide resolved

isovector reviewed Jul 5, 2019

View reviewed changes

googleson78 force-pushed the 142_lift branch 2 times, most recently from e2933ee to c824ce8 Compare July 7, 2019 19:39

googleson78 commented Jul 7, 2019

View reviewed changes

isovector approved these changes Jul 7, 2019

View reviewed changes

googleson78 force-pushed the 142_lift branch from 9a8ba8b to 838330b Compare July 8, 2019 07:00

googleson78 added 11 commits July 8, 2019 17:32

Move Polysemy.Internal.Lift to Polysemy.Lift.Type

7b5e7d4

Add Polysemy.Lift module and runLift interpreter

8773716

Add a Sandy reminder

96a5b89

Add explicit foralls and split type signature

238f210

Fix import spacing, for there is no "qualified"

b8e356a

Implement runIO in terms of runLift

82b1708

Rename Lift -> Embed

aee0326

Replace sendM with embed

b9c88c7

Add a Sandy todo for embed version

3edb5c2

Rename runEmbed and related runEmbedded (from IO)

e06f30d

* runEmbedded -> runEmbeddedInIO * runEmbed -> runEmbedded

Update cabal

df55640

googleson78 force-pushed the 142_lift branch from 838330b to df55640 Compare July 8, 2019 14:34

isovector merged commit 48b6768 into polysemy-research:master Jul 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX around Lift #161

UX around Lift #161

googleson78 commented Jul 5, 2019 •

edited

Loading

isovector left a comment

isovector Jul 5, 2019

isovector Jul 5, 2019

googleson78 Jul 6, 2019

googleson78 Jul 6, 2019

isovector Jul 6, 2019

googleson78 Jul 7, 2019 •

edited

Loading

isovector commented Jul 7, 2019 via email

googleson78 Jul 7, 2019

isovector Jul 7, 2019

googleson78 commented Jul 7, 2019

googleson78 Jul 7, 2019

isovector Jul 7, 2019

googleson78 Jul 8, 2019 •

edited

Loading

isovector Jul 8, 2019

isovector left a comment

isovector Jul 7, 2019

isovector Jul 7, 2019

isovector Jul 7, 2019

googleson78 Jul 8, 2019

isovector Jul 7, 2019

googleson78 Jul 8, 2019

isovector Jul 7, 2019

googleson78 commented Jul 11, 2019

isovector commented Jul 11, 2019 via email

isovector commented Jul 11, 2019

UX around Lift #161

UX around Lift #161

Conversation

googleson78 commented Jul 5, 2019 • edited Loading

isovector left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

isovector commented Jul 7, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 commented Jul 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isovector left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googleson78 commented Jul 11, 2019

isovector commented Jul 11, 2019 via email

isovector commented Jul 11, 2019

googleson78 commented Jul 5, 2019 •

edited

Loading

googleson78 Jul 7, 2019 •

edited

Loading

googleson78 Jul 8, 2019 •

edited

Loading