search

purescript-contrib / purescript-profunctor-lenses

Pure profunctor lenses
MIT License
144 stars 51 forks source link

Add affine traversals #112

Closed sjoerdvisscher closed 4 years ago

sjoerdvisscher commented 4 years ago

This is an attempt at reviving PR #80.

Thanks to @MonoidMusician and @LiamGoodacre for most of the code.

What I changed is using affineTraversal for most of the ix implementations and replacing maybe with maybe' to prevent unnecessary calculations in Data.Lens.At (as noted by @LiamGoodacre in #80).

thomashoneyman commented 4 years ago

I'm happy to help merge and release once @MonoidMusician and @LiamGoodacre are able to weigh in on the changes themselves.

As a note on the build -- I'm going to be updating all the -contrib libraries to use a different command to fetch the PureScript tag during the Travis build, so you may need to integrate that change before merging. Thanks for fixing it here, though!

MonoidMusician commented 4 years ago

Thanks for this, it looks good to me! Sorry for letting the other PR stall. My two minor comments are (1) maybe you want to provide a simple version of affineTraversal that works like prism' in using Maybe instead of Either, and (2) I was thinking it might be cleaner to merge At and Ix into one file so they can reuse each other’s logic (and avoid circular dependencies), but it’s not a big deal either way, and I don’t want to let the perfect get in the way of the good 👀

sjoerdvisscher commented 4 years ago

(1) This leads to the question how to call it? Also I don't think people will often implement affine traversals by hand, so there's not much need to make that easier.

(2) There's not much shared code anymore between At and Ix, since it is not very efficient to implement ix i as at i <<< _Just.

thomashoneyman commented 4 years ago

I’d like to leave this open for another few days for @LiamGoodacre or other maintainers to weigh in: if there are no objections in that time I will merge this. Thanks for your work!