Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add affine traversals #112

Merged
merged 10 commits into from
Apr 1, 2020

Conversation

sjoerdvisscher
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

(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
Copy link
Contributor

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!

@thomashoneyman thomashoneyman self-assigned this Apr 1, 2020
@thomashoneyman thomashoneyman merged commit c869e57 into purescript-contrib:master Apr 1, 2020
@MonoidMusician MonoidMusician mentioned this pull request Apr 1, 2020
@sjoerdvisscher sjoerdvisscher deleted the affine-traversal branch April 3, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants