DroidKaigi / conference-app-2018

The Official Conference App for DroidKaigi 2018 Tokyo
Apache License 2.0
1.35k stars 332 forks source link

Refactor to replace LocalDateTime into Instant. #632

Closed nukka123 closed 6 years ago

nukka123 commented 6 years ago

Issue

Overview (Required)

Currently, Session and SessionEntity class use LocalDateTime. However, since LocalDateTime does not have TimeZone, it is often misleading about the dependency of TimeZone.

I propose to replace LocalDateTime into Instant. In the javadoc of Instant, there are the following description.

This class models a single instantaneous point on the time-line. This might be used to record event time-stamps in the application.

Also, I added javadoc for parseDateString().

By refactoring, it seems to be able to delete redundant transformations such as toUnixMills() and db.entity.mapper.Converter.

What do you think?

Links

-

Screenshot

not changed.

takahirom commented 6 years ago

:cool:

takahirom commented 6 years ago

@nukka123 Do you check different timezone? ✅

nukka123 commented 6 years ago

@takahirom Thank you for comment. and, Have a nice tomorrow!

Yes. it works fine.

I have tested the case of JST(GMT+9:00) and EST(GMT-5:00). EST indicates -14 hours earlier than JST.

Expected result of session start time is as follows.

TimeZone test1 test2 test3 test4
JST(GMT+9:00) DAY1/10:00- DAY1/14:00- DAY2/10:30- DAY2/18:30-
EST(GMT-5:00) DAY1/20:00- DAY1/00:00- DAY2/20:30- DAY2/04:30-

Actual result is as follows.

TimeZone test1 test2 test3 test4
JST(GMT+9:00) no632_jst_01 no632_jst_02 no632_jst_03 no632_jst_04
EST(GMT-5:00) no632_nyt_01 no632_nyt_02 no632_nyt_03 no632_nyt_04

Actual results in "time based sessions" are as follows.

TimeZone test1 test2 test3 test4
EST(GMT-5:00) no632_t2_nyt_01 no632_t2_nyt_02 no632_t2_nyt_03 no632_t2_nyt_04
takahirom commented 6 years ago

🆒