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

Use Uuid from Kotlin standard library #758

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ykws
Copy link

@ykws ykws commented Sep 21, 2024

Hi @twyatt
Although it may already be in progress, I tried switching to Kotlin UUID.

https://kotlinlang.org/docs/whatsnew2020.html#standard-library

I have modified it so that only one @OptIn(ExperimentalUuidApi::class) annotation is needed for the file.
Please let me know any feedback.

@ykws ykws requested review from twyatt and a team as code owners September 21, 2024 03:06
@twyatt
Copy link
Member

twyatt commented Sep 21, 2024

Awesome, thanks for this!
I'll want to wait to get this in not the next release (0.35.0), but the release after that (0.36.0).
So, you may have to do some rebasing and/or conflict resolution, apologies for that.
I'll keep you posted though.

@twyatt twyatt added this to the 0.36.0 milestone Sep 21, 2024
@ykws
Copy link
Author

ykws commented Sep 21, 2024

@twyatt Thank you. I’ll leave it to you. I encountered some errors and did a force push. The CI is now showing as “Action required.” Could you please rerun it?

https://github.com/JuulLabs/kable/actions/runs/10969254146

@ykws
Copy link
Author

ykws commented Sep 21, 2024

I will check the following error.

Task :kable-core:androidApiCheck FAILED

The type of UUID might have unintentionally changed Java to Kotlin.

@twyatt
Copy link
Member

twyatt commented Sep 21, 2024

Run ./gradlew apiDump and then push changes.

@twyatt twyatt added the major Changes that should bump the MAJOR version number label Sep 21, 2024
@twyatt
Copy link
Member

twyatt commented Oct 23, 2024

@ykws when you have a chance, can you rebase this onto the latest main?

@ykws ykws marked this pull request as draft October 31, 2024 18:57
@ykws ykws marked this pull request as ready for review November 2, 2024 02:02
@ykws
Copy link
Author

ykws commented Nov 2, 2024

@twyatt Thank you for keeping me informed. Merged the main branch and fixed the build. CI passed successfully. Please review again.

@twyatt
Copy link
Member

twyatt commented Nov 5, 2024

Pretty busy on another project at the moment, but will review soon.

Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

Great job and thank you!

@twyatt twyatt changed the title Use kotlin Uuid Use Uuid from Kotlin standard library Nov 6, 2024
@twyatt
Copy link
Member

twyatt commented Nov 6, 2024

I will integrate/test this with SensorTag sample app then merge soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Changes that should bump the MAJOR version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Uuid from Kotlin standard library
3 participants