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

Replace Console with Logger almost everywhere #1393

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Replace Console with Logger almost everywhere #1393

merged 2 commits into from
Mar 8, 2023

Conversation

Maddin-M
Copy link
Contributor

@Maddin-M Maddin-M commented Mar 7, 2023

First time opening a PR on another persons repo on Github, hope I did it right. Also my (pretty strict) Lint plugin really wants to go crazy on this project 😄

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 24.48% and project coverage change: +0.01 🎉

Comparison is base (e3998b3) 51.97% compared to head (26ea828) 51.98%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1393      +/-   ##
==========================================
+ Coverage   51.97%   51.98%   +0.01%     
==========================================
  Files        1644     1644              
  Lines       99986    99997      +11     
  Branches    14591    14591              
==========================================
+ Hits        51963    51982      +19     
+ Misses      43919    43900      -19     
- Partials     4104     4115      +11     
Flag Coverage Δ
unittests 51.98% <24.48%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...mMain/kotlin/com/soywiz/korau/sound/impl/jna/AL.kt 0.00% <0.00%> (ø)
...om/soywiz/korau/sound/impl/jna/JnaSoundProvider.kt 0.00% <0.00%> (ø)
...ommonMain/kotlin/samples/MainGpuVectorRendering.kt 0.00% <0.00%> (ø)
...ge/src/commonMain/kotlin/com/soywiz/korge/Korge.kt 44.56% <0.00%> (ø)
...src/commonMain/kotlin/com/soywiz/kgl/KmlGlProxy.kt 29.97% <0.00%> (-0.04%) ⬇️
...Main/kotlin/com/soywiz/korgw/platform/INativeGL.kt 0.00% <0.00%> (ø)
...mmonMain/kotlin/com/soywiz/korio/time/TraceTime.kt 62.50% <0.00%> (ø)
...monMain/kotlin/com/soywiz/korim/format/AVIFInfo.kt 60.90% <14.28%> (+0.35%) ⬆️
.../commonMain/kotlin/com/soywiz/korim/format/EXIF.kt 67.05% <25.00%> (+0.39%) ⬆️
...kotlin/com/soywiz/korag/shader/gl/GlslGenerator.kt 68.62% <33.33%> (+0.15%) ⬆️
... and 38 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -13,7 +13,7 @@ inline fun <T : Any> traceTime(name: String, block: () -> T): T {
result = block()
}
if (traceTimes) {
Console.info("$name loaded in $time")
Logger("TraceTime").info { "$name loaded in $time" }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe @PublishedAPI internal val traceTimeLogger outside the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -163,6 +165,7 @@ private class NodeJsAsyncClient(val coroutineContext: CoroutineContext) : AsyncC
private class NodeJsAsyncServer : AsyncServer {
private val net = require_node("net")
private var server: dynamic = null
private val logger = Logger("NodeJsAsyncServer")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe inside a companion object, so the calling happens just once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (also for asyncClient)

@soywiz
Copy link
Member

soywiz commented Mar 8, 2023

Nice! Just a couple minor issues.

Glad you were able to set it up and overcome the linting that is provably OCR-ish 🤣 and make the PR. It worked well

@soywiz soywiz merged commit e10be10 into korlibs:main Mar 8, 2023
@soywiz
Copy link
Member

soywiz commented Mar 8, 2023

LGTM. Thanks! ☺️

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