Skip to content

Changes the use of Intervals/Timestamps as long into Kotlin Duration/Instant #553

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 3 commits into from
Jun 27, 2024

Conversation

Daeda88
Copy link
Contributor

@Daeda88 Daeda88 commented Jun 26, 2024

Fixes a bit of a pet peeve to me: When dealing with Intervals and Timestamps, on Kotlin we should be using Kotlin Duration and Kotlin DateTime Instant instead of using Longs.

My reasoning behind this:

  • Duration gives annotation to the meaning of a long. No longer do you need to check documentation what the long means (or check if we're using seconds or milliseconds)
  • The NSTimeInterval is in seconds while Android/JS communicate in milliseconds. This already lead to several bugs that I've solved in this PR. Using duration makes making mistakes less likely
  • If you want to set some timeout to say 3 hours, you no longer need to do calculations like 3 * 60 * 60 * 1000 as you can just pass 3.hours. This leads to better usage of the library

Daeda88 added 2 commits June 26, 2024 13:02

Verified

This commit was signed with the committer’s verified signature.
lukekarrys Luke Karrys
@nbransby
Copy link
Member

Durations aren't designed to be used as timestamps, we should probably be using https://kotlinlang.org/api/kotlinx-datetime/kotlinx-datetime/kotlinx.datetime/-instant/

@Daeda88
Copy link
Contributor Author

Daeda88 commented Jun 26, 2024

Durations aren't designed to be used as timestamps, we should probably be using https://kotlinlang.org/api/kotlinx-datetime/kotlinx-datetime/kotlinx.datetime/-instant/

Right, though I guess the ticket is also a bit of a misnomer as most of these are intervals not timestamps, for which duration is the right class.

Firebase.storage.setMaxOperationRetryTimeMillis(10000) -> Firebae.storage.setMaxOperationRetryTime(10.seconds)

to me is the main reason for changing this (well that and is confusion. In the code on master, that 10000 was not divided by 1000 so it would be set to 10000 seconds on iOS)

@Daeda88 Daeda88 changed the title Changes the use of timestamps as long into Kotlin Duration Changes the use of Intervals/Timestamps as long into Kotlin Duration/Instant Jun 26, 2024
@nbransby
Copy link
Member

Agree the timeouts should be Durations

@Daeda88
Copy link
Contributor Author

Daeda88 commented Jun 26, 2024

Ok, there was one place where Instant made more sense (in the config module). Moved to it there

@nbransby nbransby merged commit e996b7c into GitLiveApp:master Jun 27, 2024
14 checks passed
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.

None yet

2 participants