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

Optics based on kotlin.reflect #2612

Merged
merged 7 commits into from
Feb 9, 2022
Merged

Optics based on kotlin.reflect #2612

merged 7 commits into from
Feb 9, 2022

Conversation

serras
Copy link
Member

@serras serras commented Dec 24, 2021

The issue #2418 (by @magneticflux-) has given me an idea for an API for optics based on KProperty. It's a bit rough on the edges (for example, there's no way to check at compile-time that you are referring to a field in a data class, so the access may fail at runtime). On the other hand, it allows you to create optics easily for types you do not control:

data class Person(val name: String, val age: String, val friends: List<Person>)

val friendNames = Person::friends.every compose Person::name.lens
friendNames.modify(someFriends) { it.capitalize() }

The prism part is done using a combinator instance<Parent, Children>. Maybe a better name is needed, though.

@nomisRev
Copy link
Member

As a rule Arrow never exports unsafe functions from the main APIs.
I was hoping to solve this issue with a compiler plugin, but they've been on hold for a long time since Optics was released.
Soon they might become stable though, and creating optics for classes you don't own will be automatically solved as a part of that.

With that in mind, that'd also mean we'd have to deprecate this.
We could try looking into doing this with Reflekt though.

@nomisRev
Copy link
Member

Or perhaps in a second module arrow-optics-reflect so it can be opted-in.

@serras
Copy link
Member Author

serras commented Dec 24, 2021

Or perhaps in a second module arrow-optics-reflect so it can be opted-in.

I can do that, sure. The goal here was to provide a simple bridge which works for now.

@nomisRev
Copy link
Member

Yes, that makes perfect sense. We've historically done that in other projects too, there is a small reflection based module in Arrow Endpoints for Schema too but hopefully we can replace all those with nicer plugins next year 🥳

Thanks for adding this! When moved to arrow-optics-reflect, and perhaps a small addition in the docs we can merge 👍

@serras
Copy link
Member Author

serras commented Jan 10, 2022

@nomisRev I've just pushed a commit which moves this to a new arrow-optics-reflect package. If you think it's OK there, I can move on and write some documentation.

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Yes, that looks perfect @serras! Thanks for moving it 👍
Some docs would be great too ❤️

Let me know when you think this is ready, and I'll review and ✔️

Comment on lines 18 to 19
api(libs.kotlin.stdlibCommon)
implementation(libs.kotlin.stdlibJDK8)
Copy link
Member

Choose a reason for hiding this comment

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

So this exposes the API of Kotlin Std, and uses the JDK8 implementation?

I'm still not a 100% sure how this works, and what is the correct config :/
I don't we're doing different things in different modules.. but off-topic for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure 100% either. In any case, I've removed the api dependency on stdlibCommon and just left the implementation for the JDK version.

@nomisRev
Copy link
Member

nomisRev commented Feb 5, 2022

@serras should become green after merging main.

@nomisRev nomisRev requested a review from a team February 8, 2022 13:57
@nomisRev nomisRev merged commit 16d8136 into main Feb 9, 2022
@nomisRev nomisRev deleted the optics-reflection branch February 9, 2022 14:34
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