-
Notifications
You must be signed in to change notification settings - Fork 29
PT-447: Explicitly include Android variants in analysis #28
Conversation
configureAndroidProject(isAndroidApp ? project.android.applicationVariants : project.android.libraryVariants) | ||
configureAndroidProject(project.android.testVariants) | ||
configureAndroidProject(project.android.unitTestVariants) | ||
NamedDomainObjectSet<Object> variants = project.container(Object) |
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.
Why not using just Set
?
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 actually tried, but it was failing to execute addAll()
with a cryptic stacktrace that sounded like a covariance issue.
The rationale of using NamedDomainObjectSet
is that every project.android.*Variants
is already of this type, and it supports an elegant matching()
that reads better than the findAll()
provided in Collection
.
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.
Makes sense. matching
is definitely looks better.
@@ -64,7 +70,7 @@ abstract class CodeQualityConfigurator<T extends SourceTask, E extends CodeQuali | |||
} | |||
} | |||
|
|||
protected abstract void configureAndroidProject(Object variants) | |||
protected abstract void configureAndroidProject(NamedDomainObjectSet variants) |
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.
👍
private static final String DEFAULT_CHECKSTYLE_CONFIG = "checkstyle { configFile new File('${Fixtures.Checkstyle.MODULES.path}') }" | ||
private static final String DEFAULT_CHECKSTYLE_CONFIG = """ | ||
checkstyle { configFile new File('${Fixtures.Checkstyle.MODULES.path}') } | ||
""" |
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.
how come these are """
?
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.
because of the string interpolation (ie: ${...}
), you need to use GString
(quoted in ""
) instead of simple String
(using ''
)
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.
ahh didn't notice the inner ''
👍
result.buildFileUrl('reports/checkstyle/main.html'), | ||
result.buildFileUrl('reports/checkstyle/demo.html')) | ||
def checkstyleTasks = result.tasksPaths.findAll { it.startsWith(':checkstyle') } | ||
Truth.assertThat(checkstyleTasks).containsAllIn([ |
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.
awhhh yeahh 💯
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.
If this doesn't make you and @eduardb happy I really don't know what will :D
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.
from what I understand, LGTM!
general question, do you have to use strings when testing the configuration inputs?
The reason why you see these strings is because how the integration tests have been designed: each integration test creates a proper Gradle project, and the strings provided in the configuration are collated in their |
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.
👍
Scope of the PR
When applying the plugin we want to skip the creation of tasks for certain Android variants to speed up the whole analysis. After discussing this with @ouchadam and @devisnik we agreed it would be cool to have an explicit way to tell the plugin which Android variants should be considered and which shouldn't.
Considerations and implementation
Each
*Configurator
now decorates the tool specific extension with anincludeVariants
method that will receive a predicate used to decide which variant should be considered as part of the analysis. Eg:By default - for the moment - all variants will be included unless an explicit predicate is provided via
includeVariants
.Test(s) added
Few integration tests have been added to validate that tasks related to excluded variants are not created by the plugin. Incidentally some redundant integration test has been removed because their scenarios were already covered.