michaelt / streaming

An optimized general monad transformer for streaming applications, with a simple prelude of functions
BSD 3-Clause "New" or "Revised" License
104 stars 11 forks source link

`mapOf` not being exported #13

Closed echatav closed 8 years ago

echatav commented 8 years ago

I can make a PR for this; should it be exported from Streaming.Prelude only or also from Streaming?

michaelt commented 8 years ago

Right, it should just be exported from Streaming.Prelude. I think I will come up with another name so it can be exported by Streaming, maybe ofMap ...? As it is, it can't be exported there since Control.Lens has a mapOf (which is however deprecated). Streaming should not tend to overlap with stuff (except bits of FreeT and Pipes.Group). I was thinking of just writing a Bifunctor instance, in which it would just be first; the problem is that this involves piles more dependencies, though I remember hearing it was going to appear in base.

PierreR commented 8 years ago

I admit I don't really know about the subject at stake but to me a dependency on Bifunctor sounds much better than introducing the weird ofMap name ...

michaelt commented 8 years ago

I agree ofMap is absurd! Bifunctors turns out to pretty expensive dependency-wise. As things stand, the requirements are

$ cabal install streaming --dry-run
...
mtl-2.2.1
stm-2.4.4.1
transformers-compat-0.5.1.4
exceptions-0.8.2.1
mmorph-1.0.6
transformers-base-0.4.4
monad-control-1.0.1.0
lifted-base-0.2.3.6
resourcet-1.1.7.3
streaming-0.1.4.1

but with bifunctors

$ cabal install streaming bifunctors --dry-run 
...
base-orphans-0.5.4
mtl-2.2.1
stm-2.4.4.1
StateVar-1.1.0.3
tagged-0.8.3
text-1.2.2.1
hashable-1.2.4.0
transformers-compat-0.5.1.4
distributive-0.5.0.2
exceptions-0.8.2.1
mmorph-1.0.6
transformers-base-0.4.4
monad-control-1.0.1.0
lifted-base-0.2.3.6
resourcet-1.1.7.3
streaming-0.1.4.1
unordered-containers-0.2.7.0
semigroups-0.18.1
void-0.7.1
contravariant-1.4
comonad-5
bifunctors-5.3

I.e. suddenly we need text , hashable, unordered-containers. I think it is best to export mapOf from Streaming.Prelude which needs qualification anyway, but I'm not sure.

echatav commented 8 years ago

Were it not for the dependencies, I also support a Bifunctor instance. It will indeed be part of base soon so maybe hold off until then and export an instance for users of GHC 8 with some CPP for backwards compatibility.

michaelt commented 8 years ago

Oh I see it's already been around for almost a year; I was retaining the idea that it was coming in the future from too far in the past.

So maybe the best plan is to write the instance with cpp for users of base >= 4.8.0.0 and export Bifunctor(..) from Streaming, which exports a number of classes as a convenience.

michaelt commented 8 years ago

Okay, so I managed that plan, which works with ghc-7.10.x already

>>> import Streaming 
>>> import qualified Streaming.Prelude as S 
>>> first even (1:>"hi")
False :> "hi" 
>>> S.mapOf even (1:>"hi")
False :> "hi"

This seems like a reasonable arrangement. I also completed the plan of exporting two lenses _first and _second which also do this work, for what it's worth

>>> import Lens.Micro
 >>> over S._first even (1:>"hi")
 False :> "hi"
 >>>  (1:>"hi") ^. S._second
 "hi"

Thanks for the discussion!