streaming
streaming copied to clipboard
michaelt
Eq and Show instances
This is actually two separates comments bundled up into one. Since they both deal with the Eq instance, I figured that it would be best to keep them in a single issue.
The first issue is that the Eq instance for Stream is bad. Since Stream only satisfies the monad transformer laws up to observation, special care was taken all functions exported by Streaming and Streaming.Prelude to ensure that observationally equivalent values of Stream are in fact indistinguishable. The Eq instance appears to be an oversight. We can see this with the following:
>>> let x = lift (Identity ()) >> lift (Identity ())
>>> let y = lift (Identity () >> Identity ())
>>> (x >> SP.yield 'a') == (y >> SP.yield 'a')
False
This should be True. This can easily be fixed by making the implementation of == call unexposed on both sides first and then perform a normal equality check. I can PR this if you'd like.
The second issue (which is not a correctness issue but more of a clarity issue) is that both the Eq and Show instances can be improved by using higher kinded typeclasses as constraints instead of FlexibleContexts. Instead of:
(Eq r, Eq (m (Stream f m r)), Eq (f (Stream f m r))) => Eq (Stream f m r)
It would be:
(Eq r, Eq1 m, Eq1 f) => Eq (Stream f m r)
An Eq1 instance would need to be provided for Of. To keep the library compatible with transformers-0.4, some CPP would need to be used to provide implementations compatible with the old definition of these typeclasses as well. Anyway, if you're interested in using Eq1 and Show1 for the constraints, I would be happy to PR it. Also, streaming-bytestring could benefit from some similar modifications to the instances for ByteString, and I'd be happy to make those changes as well.
Oh that's true. I was thinking about the Eq and Show instances together, and as not for serious use, but just for observing the operation of the underlying type experimentally. This doesn't make any sense at all for the Eq instance though, you're right.
Pull requests would be great! I'm not totally sure I see the objection to the way the instances are constrained, using -XUndecidableInstances and this sort of ugly constraint . I notice Control.Monad.Trans.Free is using this method for Eq on hackage, but the other for Show. Meanwhile everything is undergoing change though https://github.com/ekmett/free/pull/142
I think Eq1 and friends just have the advantage of requiring less type extensions.
On Thu, 9 Mar 2017, 2:51 pm michaelt, [email protected] wrote:
Oh that's true. I was thinking about the Eq and Show instances together, and as not for serious use, but just for observing the operation of the underlying type experimentally. This doesn't make any sense at all for the Eq instance though, you're right.
Pull requests would be great! I'm not totally sure I see the objection to the way the instances are constrained, using -XUndecidableInstances and this sort of ugly constraint . I notice Control.Monad.Trans.Free is using this method for Eq on hackage, but the other for Show. Meanwhile everything is undergoing change though ekmett/free#142 https://github.com/ekmett/free/pull/142
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/michaelt/streaming/issues/26#issuecomment-285372119, or mute the thread https://github.com/notifications/unsubscribe-auth/AABRjrm2_zaBHIMAUmBnxlP3XKg7gklZks5rkBHUgaJpZM4MXk_n .
Yeah, the main benefit is just needing fewer extensions. I'll try to get together a PR this weekend.
Eq1 is really overkill here, since you only apply it at one type. Using it means only working with Eq1 instances, which are less common than the corresponding Eq instances. I don't really think it's worth avoiding the extension.
