Make HasStream a super class of HasWriter
#64
Conversation
| deriving (Functor, Applicative, Monad, MonadIO, PrimMonad) | ||
| instance HasWriter tag (DList a) m => HasStream tag a (StreamDList m) where | ||
| yield_ _ = coerce @(a -> m ()) $ tell @tag . DList.singleton | ||
| instance HasStream tag (DList a) m => HasStream tag a (StreamDList m) where |
There was a problem hiding this comment.
There's no reason to pull in all of HasWriter to discharge this.
There was a problem hiding this comment.
The haddocks do not really represent what is happening here anymore, do they? In the case of HasWriter tag (DList a) it was clear that the dlists would be combined using their monoid instance, but with HasStream tag (DList a) this is no longer necessarily the case. It could be that we're using S.Stream (Of (DList a)) underneath. In that case we could end up with, say, a list of singleton dlists if we choose to run the stream that way.
|
|
||
| -- | Accumulate streamed values with their own monoid. | ||
| newtype StreamLog m (a :: *) = StreamLog (m a) | ||
| deriving (Functor, Applicative, Monad, MonadIO, PrimMonad) |
There was a problem hiding this comment.
With this change, WriterLog needs an instance for HasStream, so the strategy becomes StreamLog and goes here. There is a WriterLog alias for back compat.
| -- over | ||
| -- | ||
| -- > deriving (HasStream tag w) via | ||
| -- > Lift (SomeTrans (StreamLog (MonadState SomeStateMonad))) |
There was a problem hiding this comment.
This issue already existed: Lift (StreamDList (WriterLog (... SomeStateMonad))) was already possible, whereas StreamDList (WriterLog (... (Lift SomeStateMonad))) would be the preferred alternative. That said, with new StreamLog, less indirection is needed to create the same scenario. Given that, I decided to copy the warning from Capability.Writer.Discouraged, but not move the instance to a Capability.Stream.Discouraged as there's plenty of non-state-based ways to implement HasStream.
aherrmann
left a comment
There was a problem hiding this comment.
Thanks! It's taking me a while to grok this change. But, I think I'm seeing what you're getting at.
Though, I think this deserves some more documentation. I'd also love an example that makes use of this super class relationship, if you can think of something.
Here's what confused me about this. Other future users might be similarly confused.
My intuition for HasStream tag w m is that it is something that talks about individual values of type w where each yield tosses a new value down the chute, such as Stream (Of w).
On the other hand, my intuition for HasWriter tag w m is that it is something that talks about one value of type w where every tell makes the value grow bigger by its monoid instance.
These intuitions collide in HasStream tag w m => HasWriter tag w m.
Of course, an instance of HasStream tag w m that collapses all w into one is perfectly valid.
It's just not immediately obvious.
| deriving (Functor, Applicative, Monad, MonadIO, PrimMonad) | ||
| instance HasWriter tag (DList a) m => HasStream tag a (StreamDList m) where | ||
| yield_ _ = coerce @(a -> m ()) $ tell @tag . DList.singleton | ||
| instance HasStream tag (DList a) m => HasStream tag a (StreamDList m) where |
There was a problem hiding this comment.
The haddocks do not really represent what is happening here anymore, do they? In the case of HasWriter tag (DList a) it was clear that the dlists would be combined using their monoid instance, but with HasStream tag (DList a) this is no longer necessarily the case. It could be that we're using S.Stream (Of (DList a)) underneath. In that case we could end up with, say, a list of singleton dlists if we choose to run the stream that way.
| -- prop> pass @t (tell @t w >> pure (a, f)) = tell @t (f w) >> pure a | ||
| -- prop> writer @t (a, w) = tell @t w >> pure a | ||
| class (Monoid w, Monad m) | ||
| class (Monoid w, Monad m, HasStream tag w m) |
There was a problem hiding this comment.
I think HasStream tag w m => HasWriter tag w m deserves a good bit of additional documentation. To me this is far from obvious. (See overall review comment).
|
Thanks @aherrmann. I'm quite willing to add all the docs you or anyone wants for these changes; I definitely agree these changes are quite surprising up front for people used to the MTL/seeing
My view is sort of syntactic. |
Thank you! Yes, I think this is a good addition to |
|
OK added some documentation. |
|
@Ericson2314 Sorry for leaving this hanging for so long. Thanks for the added documentation! Looks good to me. I noticed that the examples needed fixing. I added the corresponding changes. |
|
Oh, right. Thanks for fixing those! |
Does the good half of #63.
I went ahead to give that idea a spin, and found a few interesting things in the process. Will comment in line notes.