-
Notifications
You must be signed in to change notification settings - Fork 1
Change public API: rename onTransformInterval callback, add support for SpannableString
#35
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a9d96e1
Rename OnTransformInterval callback to OnApplyIntervalTransformation …
st235 b50b8ad
Add support for SpannableString for non-compose applications
st235 7bd460c
OnApplyIntervalTransformation now provides endsAt instead of length a…
st235 c4c786a
Update public API
st235 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
typealiasand in the public API it is substituted withLkotlin/jvm/functions/Function4;. So this name does not really exist in bytecode.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.0then 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 🤗
There was a problem hiding this comment.
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.
Yeah, we have no obligation for now to keep the API intact, especially, while we are still shaping it.
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.