Skip to content

Conversation

@st235
Copy link
Collaborator

@st235 st235 commented Aug 28, 2025

No description provided.

@st235 st235 requested a review from mezpahlan August 28, 2025 14:07
@st235 st235 changed the title Ad/change public api Change public API: rename onTransformInterval callback, add support for SpannableString Aug 28, 2025
@st235 st235 force-pushed the ad/change_public_api branch from 594f46a to 731a233 Compare August 28, 2025 14:12
@st235 st235 requested a review from jordicollmarin August 28, 2025 14:13
@st235 st235 force-pushed the ad/change_public_api branch from 731a233 to 0e8e6fe Compare August 28, 2025 14:17
@st235 st235 force-pushed the ad/change_public_api branch from 0e8e6fe to c4c786a Compare August 28, 2025 14:19
Comment on lines -16 to +17
typealias OnTransformInterval<T> = T.(id: String, startsAt: Int, length: Int) -> Unit
typealias OnApplyIntervalTransformation<T> = T.(id: String, startsAt: Int, endsAt: Int) -> Unit
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite sure why this change doesn't appear in the API diff? Why isn't this a breaking change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I was also thinking about it. My guess because this is typealias and in the public API it is substituted with Lkotlin/jvm/functions/Function4;. So this name does not really exist in bytecode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you able to do some checks as to whether this is breaking or not? It's cool if it's not a breaking change. I just haven't seen this before. So don't know.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The check runs automatically on CI. I was able to run it locally and it is passing: so I believe this is not a breaking change. I agree this is weird as this is a public name though I believe the name is inlined that's why it is not picked up in the resulting API file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to be a pain but whilst this might not break binary compatibility it does break source compatibility.

Given you've listed this as alpha, and a very early alpha as well, I think that helps us here.

Screenshot_20250828-203318.png

What I think we should do is for now simply call out the change in a change log before publishing a new version. Ping me tomorrow I can help with that.

I think we should also decide together with @jordicollmarin how we want to evolve the API here in this project. I'm keen that we preserve binary and source compatibility but I understand that adds complexity. Again, this library still being an alpha release buys us time 👍.

When the API stabilises to the point where we release a 1.0.0 then we really need to be careful.... even about renaming parameters.

https://kotlinlang.org/docs/api-guidelines-backward-compatibility.html

But I think we're ok for now. Great job 🤗

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mezpahlan no worries, that's definitely a valid point. Let's catch up tomorrow on the evolution of the API.

Given you've listed this as alpha, and a very early alpha as well, I think that helps us here.

Yeah, we have no obligation for now to keep the API intact, especially, while we are still shaping it.

What I think we should do is for now simply call out the change in a change log before publishing a new version.

It would be really cool if we can generate a changelog automatically.

Let's think about possible changes tomorrow as I have one potential API change in mind that I think we may want to implement.

@st235 st235 requested a review from mezpahlan August 28, 2025 15:19
@st235 st235 merged commit 06b8f2f into main Aug 28, 2025
4 checks passed
@st235 st235 deleted the ad/change_public_api branch August 28, 2025 20:22
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