Ignore composables that emit content in their own window. #110
Conversation
|
|
PaulWoitaschek
changed the title
|
CLA Assistant Lite bot All Contributors have signed the CLA. If the commit check is not passing, a maintainer must go the Checks tab of this PR and rerun the GitHub Action. |
@PaulWoitaschek could you reply w/ a comment with the above text. We actually have two CLA bots, weirdly enough... |
|
I have read the CLA Document and I hereby sign the CLA |
| get() { | ||
| return if (isComposable) { | ||
| sequence { | ||
| suspend fun SequenceScope<KtCallExpression>.scan(current: PsiElement) { |
There was a problem hiding this comment.
can this be made tailrec? if not, then using a queue to make it iterative?
There was a problem hiding this comment.
Why would that be necessary? This runs on user written code and I don't see where it could produce a stack overflow.
There was a problem hiding this comment.
Twitter for Android has several million LOC, and thousands of modules. It is the main target for us to run these rules. We've seen that not being conservative with memory causes heap to overflow our CI in the past, so any savings we can do here are worthwhile for us.
There was a problem hiding this comment.
Sure, but ktlint runs only on a single file. So even if you had very bad code standards and one function referencing 100 composables, that still would be premature optimization.
There was a problem hiding this comment.
If you count that it is filtering by KtCallExpressions (so not only constrained to Composables), from experience (e.g. very, very similar code to this that had to be changed, and it was targeting individual files too) this can be problematic, even if it sounds like premature optimization. Some of these files are insanely big, and we don't have the numbers now after last Friday to address the necessary refactors anymore.
Not trying to be obtuse here, just cautious 😢
There was a problem hiding this comment.
All righty, even though I think it's premature optimization, here comes the tailrec magic 🧙♀️
| if (klass.isInstance(current)) { | ||
| yield(current as T) | ||
| if (current is T) { | ||
| yield(current) | ||
| } | ||
| queue.addAll(current.children) | ||
| } | ||
| } | ||
|
|
||
| inline fun <reified T : PsiElement> PsiElement.findDirectFirstChildByClass(): T? { |
There was a problem hiding this comment.
will this need to be reified then? (same for the others)
There was a problem hiding this comment.
Yes else the is check can not work due to type erasure.
...s/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposeModifierMissingCheckTest.kt
Outdated
Show resolved
Hide resolved
...s/detekt/src/test/kotlin/com/twitter/compose/rules/detekt/ComposeModifierMissingCheckTest.kt
Outdated
Show resolved
Hide resolved
...s/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposeModifierMissingCheckTest.kt
Outdated
Show resolved
Hide resolved
...s/ktlint/src/test/kotlin/com/twitter/compose/rules/ktlint/ComposeModifierMissingCheckTest.kt
Outdated
Show resolved
Hide resolved
|
Looks very nice, left a couple of comments, happy to re-review later |
Co-authored-by: Nacho Lopez <nacho@nlopez.io>
Fixes #109