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.

Sign up for GitHub

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

Jump to bottom

Update ViewModelInjectionDetector to support a lint option #53

Merged

ZacSweers merged 5 commits into slackhq:main from WhosNickDoglio:ndoglio-ViewModelInjection-LintOption

Feb 18, 2023

Contributor

WhosNickDoglio commented Feb 10, 2023 •

edited

Loading

Replaces #46

WhosNickDoglio force-pushed the ndoglio-ViewModelInjection-LintOption branch from 24a815e to 2906193 Compare

February 11, 2023 16:15

Collaborator

ZacSweers commented Feb 14, 2023

@WhosNickDoglio this is still in draft, did you want to ping us when it's ready for review?

Contributor Author

WhosNickDoglio commented Feb 14, 2023

@ZacSweers Yeah sorry, I threw something together quick and was going to spend some time on it this weekend but got busy, I plan to finish it tomorrow and will ping you when it's ready 👍

WhosNickDoglio force-pushed the ndoglio-ViewModelInjection-LintOption branch from 2906193 to 3064b71 Compare

February 15, 2023 11:51

WhosNickDoglio commented

View reviewed changes

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated Show resolved Hide resolved

WhosNickDoglio marked this pull request as ready for review

February 15, 2023 12:24

Contributor Author

WhosNickDoglio commented Feb 15, 2023

@ZacSweers This is ready for review whenever you have time!

ZacSweers reviewed

View reviewed changes

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated Show resolved Hide resolved

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated Show resolved Hide resolved

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated

Comment on lines 75 to 76

		KnownViewModelFactories.contains(it.calleeExpression?.text) \|\|
		allowedNames.value.contains(it.calleeExpression?.text)

Collaborator

ZacSweers Feb 15, 2023

Let's merge the known and allowed factories into one set to avoid double checks. Let's also use the in operator

WhosNickDoglio force-pushed the ndoglio-ViewModelInjection-LintOption branch from 40e94b6 to 9419761 Compare

February 15, 2023 17:51

ZacSweers reviewed

View reviewed changes

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt

@@ @@ -8,15 +8,28 @@ import com.android.tools.lint.detector.api.Issue @@
               import com.android.tools.lint.detector.api.JavaContext
               import com.android.tools.lint.detector.api.Severity
               import com.android.tools.lint.detector.api.SourceCodeScanner
+              import com.android.tools.lint.detector.api.StringOption
               import org.jetbrains.kotlin.psi.KtCallExpression
               import org.jetbrains.kotlin.psi.KtFunction
               import org.jetbrains.kotlin.psi.KtProperty
               import slack.lint.compose.util.*

Collaborator

ZacSweers Feb 15, 2023

note for myself - we shouldn't have star imports, wonder why spotless isn't catching this 🤔

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated

                     .flatMap { property ->
                       property
                         .findDirectChildrenByClass<KtCallExpression>()
-                        .filter { KnownViewModelFactories.contains(it.calleeExpression?.text) }
+                        .filter {
+                          val allFactoryNames = KnownViewModelFactories + allowedNames.value

Collaborator

ZacSweers Feb 15, 2023

Can you move this higher up? Doesn't need to be re-computed on each call

WhosNickDoglio and others added 4 commits

February 15, 2023 16:21


          Update ViewModelInjectionDetector to support a lint option

de606d8


          Update compose-lint-checks/src/main/java/slack/lint/compose/ViewModel…

29f851b

…InjectionDetector.kt

Co-authored-by: Zac Sweers <pandanomic@gmail.com>


          Simplify filtering allowed and known ViewModel factory functions

a86a5d8


          Avoid unnecessarily creating a Set on each property checked

bf8422e

WhosNickDoglio force-pushed the ndoglio-ViewModelInjection-LintOption branch from 37ba043 to bf8422e Compare

February 15, 2023 21:22

ZacSweers reviewed

View reviewed changes

compose-lint-checks/src/main/java/slack/lint/compose/ViewModelInjectionDetector.kt Outdated

+                  internal val ALLOW_LIST =
+                    StringOption(
+                      "allowed-viewmodel-injection",

Collaborator

ZacSweers Feb 16, 2023

nit: since it's not really an allow-list, let's improve the semantics. Maybe USER_FACTORIES and viewmodel-factories?

Contributor Author

WhosNickDoglio Feb 16, 2023


          Use more accurate name for user added viewModel factory list

d39522a

ZacSweers approved these changes

View reviewed changes

Collaborator

ZacSweers left a comment

Thanks!

ZacSweers added this pull request to the merge queue

Merged via the queue into slackhq:main with commit 00b3562

WhosNickDoglio deleted the ndoglio-ViewModelInjection-LintOption branch

February 19, 2023 02:23

jakoss commented Mar 9, 2023

@WhosNickDoglio Do you have example how user can set this parameter? I'm trying to find some way to pass custom option to lint but i think i'm missing something

Contributor Author

WhosNickDoglio commented Mar 9, 2023

In your lint.xml file you would add this:

<issue id="ComposeViewModelInjection">
    <option name="viewmodel-factories" value="ViewModelFactory1,ViewModelFactory2, etc" />
</issue>

jakoss commented Mar 9, 2023

@WhosNickDoglio Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment