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

Migrate to kotlinx-datetime #4471

Merged
merged 9 commits into from
Oct 18, 2022

Conversation

YoshiRulz
Copy link
Contributor

@YoshiRulz YoshiRulz commented Oct 7, 2022

resolves #4240

On the whole this was rather easy as the kotlinx-datetime API seems to be based on Java's.

Copy link
Member

@westnordost westnordost 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 so far! I was puzzled first why only the imports changed, but saw that you solved this entirely through extension functions, mirroring the java.time API.

Instant.now().toEpochMilli() should be the same as System.currentTimeMillis(), right? The latter does not even need an explicit import because it is in java.lang but at the same time it is not cross-platform for Kotlin. So any currentTimeMillis() need replacement too.

Maybe it is worth just defining a public function called currentEpochMilliseconds() or something that uses kotlinx-time?

Did you notice any (other) shortcuts that could be taken? E.g. any usages of the whole date time stuff that is always the same and could just be shortened to use one extension or static public function instead?

@YoshiRulz
Copy link
Contributor Author

Anything else before I start on your cleaner API suggestions?

@westnordost
Copy link
Member

Maybe have the extension function for the track name private in the TracksApiImpl but otherwise, go ahead!

@westnordost
Copy link
Member

westnordost commented Oct 8, 2022

reading vertically, through the changes, I think it is not a good idea to use toString in code because it is not explicit what format exactly is expected at call site and hence practically undocumented except for some single unit test somewhere. When there is an explicit toCheckDateString, it can have a documentation comment that describes exactly how the output will look.

In case of refactor, it is also hard to search for it because that method is defined on Object / Any.

Generelly, I never trust toString to have a certain output or that that certain output may not change at any time. As far as I know, toString is meant to be a string representation for e.g. logs and debugging in Java. (That's why there is MyEnumConstant.name next to MyEnumConstant.toString, I figure)

@westnordost
Copy link
Member

Ping me if you think this is ready for 2nd review, I didn't read through it yet and I don't know if you are still working on something or not.

@YoshiRulz
Copy link
Contributor Author

Rebased and force pushed (backup). @westnordost I'm done with changes

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Review complete! The shortcuts look really good!

Two general notes:


I somewhat dislike Kotlin's measureTimeMillis (and measureTimedValue) (in production code) because it makes it look like the measuring of the time is the main event.

To me, it looks a bit like having a method like getSomeData actually not only return some data, but also do other things, i.e. have side-effects. (Such functions are a commonly known anti-pattern.)

Instead, the measuring of the time is the side effect, not the work done in-between the measuring. This is why I have abstained from using measureTimeMillis so far, except maybe in some test code where the main thing actually is measuring the time of operations not the operations themselves.

Further (minor) reasons why I dislike measureTimeMillis:

  • it adds an (unnecessary) level of indentation to the code. I do find code with less indentation easier to read.
  • it adds a scope. I.e. variables declared within that scope are not accessible to the outside. IMO there is no good reason for that
  • such Kotlin inline/block functions are always a bit opaque regarding what is this now (and whether there is an it. One would need to look into the documentation for each of these. This is a problem for all these functions generally of course, but this is why I try to reduce usage of such functions or delegate to declared functions

Regarding replacement of max(0, value) with value.coerceAtLeast(0L) (etc.):

Why?

I always found the coerce... functions less expressive but maybe just because I am used to min and max from other programming languages. The name is also slightly confusing, as there exists a concept of "type coercion" in other languages which however is completely unrelated to these functions.

@YoshiRulz
Copy link
Contributor Author

I forgot to mention that I also replaced a few System.currentTimeMillis calls in androidTest when rebasing. I presume that needs to run on Android, how do I do that?

@westnordost
Copy link
Member

I forgot to mention that I also replaced a few System.currentTimeMillis calls in androidTest when rebasing. I presume that needs to run on Android, how do I do that?

Do you have Android Studio installed? You can run it in an emulator. Add/start/manage emulators via this button

image

Otherwise, you can also connect your Android phone and run the tests there. But this requires a few additional steps (turn on developer mode on the phone, turn on USB debugging on the phone, maybe installing some drivers that allow your computer to see the phone, accepting your computer as trusted on your phone)

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Looks really nice!

@YoshiRulz
Copy link
Contributor Author

My phone is set up to launch and debug, but I'm running the tests with :app:testDebugUnitTest and that doesn't include a deploy at all.

Am I good to do a final rebase now? It looks like there's a new conflict to resolve as well.

@westnordost
Copy link
Member

My phone is set up to launch and debug, but I'm running the tests with :app:testDebugUnitTest and that doesn't include a deploy at all.

Hmm, not sure. I usually always run the test in the UI, i.e.

image

But I'd have thought that it does exactly the same. Don't worry about it, I will test it after the merge.

I resolved the conflict.

@westnordost westnordost merged commit a846975 into streetcomplete:master Oct 18, 2022
@westnordost
Copy link
Member

I ran the tests on Android, all clear

@FloEdelmann FloEdelmann added the hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event label Oct 20, 2022
@YoshiRulz YoshiRulz deleted the kotlify-datetime branch October 20, 2022 23:58
@westnordost westnordost mentioned this pull request Dec 20, 2023
@FloEdelmann

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

Migrate from java.time to kotlinx-datetime
3 participants