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

feat(abg)!: replace data class perks by code generation in bindings #1644

Open
wants to merge 1 commit into
base: vampire/binding-version-v2
Choose a base branch
from

Conversation

Vampire
Copy link
Collaborator

@Vampire Vampire commented Sep 6, 2024

Fixes #1629

Copy link
Collaborator Author

Vampire commented Sep 6, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Vampire Vampire force-pushed the vampire/no-data-bindings branch 2 times, most recently from fcb93fe to f2e6c05 Compare September 6, 2024 22:12
@Vampire Vampire changed the base branch from vampire/versioned-routes to vampire/binding-version-v2 September 8, 2024 08:11
@Vampire Vampire force-pushed the vampire/no-data-bindings branch 2 times, most recently from 26ff564 to 915be83 Compare September 8, 2024 15:23
docs/user-guide/using-actions.md Outdated Show resolved Hide resolved
- The `equals` method will work like before.
- The `hashCode` method will work like before.
- The `toString` method will work like before.
- Destructuring assignments are not possible anymore as this will likely break silently or during
Copy link
Member

Choose a reason for hiding this comment

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

I initially wanted to ask why this change is made under v2 of the bindings server, and it's probably because of this remark about destructuring. I think it's enough to provide component1...componentN functions, then destructuring should work, and we wouldn't have to bump the route version just yet.

Copy link
Member

Choose a reason for hiding this comment

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

My plan is to revisit our discussion we had on Slack to ensure that there's no simpler option to achieve what we want. Sometimes ideas come after time 😄

Copy link
Member

Choose a reason for hiding this comment

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

Let's see if someone has some interesting take on this: https://kotlinlang.slack.com/archives/C8C4JTXR7/p1729233924861659

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's enough to provide component1...componentN functions, then destructuring should work, and we wouldn't have to bump the route version just yet.

It would not be, as said on Slack.
It was intentional I left them out.
And additionally the copy method now also enforces named-arguments which is also a breaking change.
So to replicate here too, the goal here was not to somehow make it compatible with the upcoming Kotlin versions, but to improve the API to the originally intended way.
That this also fixes the copy visibility thing is merely a by-product and just was the trigger to finally have a look at it.

@Vampire Vampire requested a review from krzema12 October 27, 2024 03:56
@Vampire Vampire force-pushed the vampire/no-data-bindings branch 2 times, most recently from e286a75 to 9142ab8 Compare October 28, 2024 21:20
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.

2 participants