search

purescript-contrib / purescript-profunctor-lenses

Pure profunctor lenses
MIT License
144 stars 52 forks source link

Remove Index constraint from At? #23

Closed pkamenarsky closed 8 years ago

pkamenarsky commented 8 years ago

I have no idea about the theoretical foundations, but is the Index constraint on all At instances really necessary? It isn't used anywhere in the At instances in profunctor-lenses at least.

garyb commented 8 years ago

Dropping the constraint seems reasonable to me. Anyone know if it's required for some other reason? @paf31 / @zrho?

zrho commented 8 years ago

With at there is a canonical implementation of ix: ix i = at i <<< _Maybe. While the constraint isn't strictly necessary, I would find it surprising to encounter an instance of At that doesn't implement Index. What's your motivation for removing the constraint? On 15 Feb 2016 4:35 pm, "Gary Burgess" notifications@github.com wrote:

Dropping the constraint seems reasonable to me. Anyone know if it's required for some other reason? @paf31 https://github.com/paf31 / @zrho https://github.com/zrho?

— Reply to this email directly or view it on GitHub https://github.com/purescript-contrib/purescript-profunctor-lenses/issues/23#issuecomment-184256502 .

zrho commented 8 years ago

_Just of course, instead of maybe. On 15 Feb 2016 5:26 pm, "Lukas Heidemann" lukasheidemann@gmail.com wrote:

With at there is a canonical implementation of ix: ix i = at i <<< _Maybe. While the constraint isn't strictly necessary, I would find it surprising to encounter an instance of At that doesn't implement Index. What's your motivation for removing the constraint? On 15 Feb 2016 4:35 pm, "Gary Burgess" notifications@github.com wrote:

Dropping the constraint seems reasonable to me. Anyone know if it's required for some other reason? @paf31 https://github.com/paf31 / @zrho https://github.com/zrho?

— Reply to this email directly or view it on GitHub https://github.com/purescript-contrib/purescript-profunctor-lenses/issues/23#issuecomment-184256502 .

pkamenarsky commented 8 years ago

It seems to me that in order to implement an Index instance, one has to use wander from the internal module Data.Lens.Internal.Wander. Is this the case?

garyb commented 8 years ago

That might well be the case. Although Wander is "Internal" it's not internal in the sense that it's supposed to be private to purescript-profunctor-lenses, it's just that it probably doesn't have much use outside of a lens context. It's totally fine to define things in terms of wander.

pkamenarsky commented 8 years ago

Oh ok, then I'm fine with defining an Index instance in order to fulfil the At constraints.