-
-
Notifications
You must be signed in to change notification settings - Fork 783
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
Implement the rule to check if dispatchers are injectable #4222
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4222 +/- ##
============================================
+ Coverage 84.19% 84.32% +0.12%
- Complexity 3233 3249 +16
============================================
Files 468 469 +1
Lines 10185 10186 +1
Branches 1786 1783 -3
============================================
+ Hits 8575 8589 +14
+ Misses 671 658 -13
Partials 939 939
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff :)
...-coroutines/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/coroutines/InjectDispatcher.kt
Outdated
Show resolved
Hide resolved
…etekt/rules/coroutines/InjectDispatcher.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>
dispatcherNames: | ||
- 'IO' | ||
- 'Default' | ||
- 'Unconfined' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the use case to allow to configure this? Shouldn't we just check all the usages of Dispatchers.*
? If this is because you can change the Main
and the Immediate
maybe a boolean
flag (disable by default) could be a better fit for discoberavility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispatchers.Main is generally okay because we have Dispatchers.setMain()/resetMain() available in test (https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-test/)
Additionally, if users define their own Dispatchers, this list is necessary for specifying their customized dispatcherNames
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispatchers.Main is generally okay because we have Dispatchers.setMain()/resetMain()
It's possible to change them, but I think that adding Main
is a safer default. If someone is OK using those function it can remove it. This kind of test functions aren't very good. If you forget to use resetMain
you can change the behaviour of other tests.
For example JodaTime had DateTimeUtils.setCurrentMillisFixed
but it was removed in java.time
. Now you are forced to inject a Clock
if you want to test it. I think that we should add Main
but default for the same reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jetbrain announces its plan to improve kotlin coroutin test https://youtrack.jetbrains.com/issue/KT-46783.
On the other hand, [Dispatchers.Main] is used in common androidx libraries:
- Paging3: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:paging/paging-common/src/main/kotlin/androidx/paging/PagingDataDiffer.kt?q=Dispatchers.Main&ss=androidx
- LiveData: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:lifecycle/lifecycle-livedata-ktx/src/main/java/androidx/lifecycle/CoroutineLiveData.kt?q=Dispatchers.Main&ss=androidx&start=61
- Lifecycle: https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:lifecycle/lifecycle-runtime-ktx/src/main/java/androidx/lifecycle/RepeatOnLifecycle.kt?q=Dispatchers.Main&ss=androidx&start=91
Since most UI applications do have a single Main
thread, we are more likely to get complaints if we add Main
as the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't completely agree but we could move this to an issue. There's no need to block the merging for this.
This resolves #4175.