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 kotlinx.datetime for time handling #272

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Conversation

zsmb13
Copy link
Contributor

@zsmb13 zsmb13 commented Feb 11, 2025

No description provided.

@zsmb13 zsmb13 force-pushed the kotlinx-datetime branch 2 times, most recently from 4e46b1f to f5120a1 Compare February 11, 2025 10:43
Base automatically changed from screens/schedule-3 to kotlinconf-2025 February 11, 2025 10:48
@zsmb13 zsmb13 force-pushed the kotlinx-datetime branch 9 times, most recently from 228ef82 to b917dbd Compare February 11, 2025 12:48
@zsmb13 zsmb13 changed the title wip kotlinx.datetime, please ignore Use kotlinx.datetime for time handling Feb 11, 2025
@zsmb13 zsmb13 requested review from e5l and kropp February 11, 2025 13:36
@zsmb13 zsmb13 marked this pull request as ready for review February 11, 2025 13:36
Comment on lines +25 to +33
val isLightning: Boolean = endsAt - startsAt <= LIGHTNING_TALK_LIMIT
}

val SessionCardView.isLive get() = state == SessionState.Live
val SessionCardView.isUpcoming get() = state == SessionState.Upcoming
val SessionCardView.isPast get() = state == SessionState.Past

val Session.isLightning: Boolean get() = endsAt.timestamp - startsAt.timestamp <= 15 * 60 * 1000
val Session.isLightning: Boolean
get() = endsAt - startsAt <= LIGHTNING_TALK_LIMIT
Copy link
Member

Choose a reason for hiding this comment

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

minor: isLightning check is duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this both on Session and SessionCardView, I think with the limit variable extracted at least it's alright like this

Comment on lines +18 to +19
import java.time.Clock
import java.time.LocalDateTime
Copy link
Member

Choose a reason for hiding this comment

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

Is java.time still needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The backend code still uses java.time APIs in a bunch of places, I tried touching as little of the backend code as possible. Only edited the parts that were tightly coupled to the shared code.

We could rewrite the backend to be all kotlinx.datetime too, but it's not strictly necessary. Just wanted to have these types on the client side instead of using the Ktor GMTDate type everywhere.

@e5l to review the backend changes though, and whether we want more of them.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to use kotlinx.datetime if it's possible

Copy link
Member

Choose a reason for hiding this comment

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

Okay, then I think it is fine to merge this PR as is and log an issue for backend refactoring.

Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +18 to +19
import java.time.Clock
import java.time.LocalDateTime
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to use kotlinx.datetime if it's possible

@zsmb13 zsmb13 merged commit 85a1a5e into kotlinconf-2025 Feb 13, 2025
5 checks passed
@zsmb13 zsmb13 deleted the kotlinx-datetime branch February 13, 2025 07:26
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