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

Implement Lens directly on a pair #1654

Merged
merged 4 commits into from
Apr 14, 2021
Merged

Conversation

maan2003
Copy link
Collaborator

@maan2003 maan2003 commented Mar 17, 2021

I see no reason not to implement it directly 🤷 and it feels more intuitive

@maan2003
Copy link
Collaborator Author

Maybe we should implement Lens for other tuples as well.

@cmyr
Copy link
Member

cmyr commented Mar 18, 2021

@derekdreery does this make sense to you? I think so, but curious if you had a more specific need that required Tuple2 to be a named type?

I've run into this before; I think it does make sense to have lenses on the tuples, maybe from rank 1-8? I think an 8-tuple is a horrible idea but I think it's okay as an upper bound..

@maan2003
Copy link
Collaborator Author

maan2003 commented Apr 8, 2021

@cmyr @derekdreery can we get this done?

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, happy to see this merged.

@SecondFlight
Copy link
Collaborator

@maan2003 Just noticed this doesn't have a matching changelog entry. Just want to make sure you have the chance if you forgot.

@maan2003 maan2003 requested a review from cmyr April 12, 2021 18:15
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@maan2003 maan2003 merged commit ac3e0a6 into linebender:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants