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

Avoid using ExperimentalTime API #1330

Closed
wants to merge 3 commits into from
Closed

Avoid using ExperimentalTime API #1330

wants to merge 3 commits into from

Conversation

pubiqq
Copy link

@pubiqq pubiqq commented Apr 17, 2022

@arnaudgiuliani
Copy link
Member

I agree with you, this time API is breaking updates since too much time. For 3.2 we plan to rework this also, to avoid such breaking.

Let me check how our solution is going with yours.

@arnaudgiuliani arnaudgiuliani added this to the 3.2.0 milestone Apr 22, 2022
@arnaudgiuliani arnaudgiuliani added important 🔥 status:checking currently in analysis - discussion or need more detailed specs core labels Apr 22, 2022
@pubiqq
Copy link
Author

pubiqq commented Apr 22, 2022

Sure. Feel free to ask me if you have any questions about the changes made.

- Don't refer to an unstable API in the comments
- Remove TODO info from doc comments
@arnaudgiuliani
Copy link
Member

I've ported some of your code (JS/Native Nanoseconds util) - 0a10adc

It works better now 💪

@arnaudgiuliani
Copy link
Member

I close this, as I believe I have everything we need. Your feedback is welcome @pubiqq

@pubiqq
Copy link
Author

pubiqq commented Apr 28, 2022

Your feedback is welcome

Well...

  • self is not defined in Node.js, so your change doesn't work properly.
  • measureDuration and measureTime are the same thing, just one function is marked with opt-in @KoinInternalApi, and the other is not, something obviously superfluous.
  • Pair measureDurationForResult/measureTimedValue has the same problem + measureDurationForResult does redundant actions (destructures the result value and creates the same one).
  • performance.now() is available in WebView for Android SDK 19 and above, but for koin-android you declare support for Android SDK 14 and above. It may seem it's about two different things, but since you declare support for Android SDK 14, you admit that such devices exist, so you need to ensure that the library works on them as well.
  • File naming leaves much to be desired, like KoinPlatformTimeTools class (ok, assume) inside PlatformTimeToolsJVM.kt file (you are already inside the jvm target, you don't need to mention it again with a suffix) inside mp folder (in short, it's weird to have a "multiplatform" folder inside a folder associated with a specific platform) inside jvm target.
  • The code is not formatted everywhere, use the "Reformat Code" command or the appropriate keyboard shortcut (for Android Studio: Ctrl + Alt + L on Windows and Linux, ⌥⌘L on Mac). If you constantly forget about it, or you don't want to do it manually, you can configure the environment to do this before commit (for Android Studio: "File" -> "Settings" -> "Version Control" -> "Commit" -> group "Before Commit" -> select the necessary actions: reformat, cleanup and so on). Also, you can delegate this work to your CI platform (there is no specific advice here, it all depends on the CI platform and your development process).

To be honest, I would still recommend using my PR. 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core important 🔥 status:checking currently in analysis - discussion or need more detailed specs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants