fix Consider using 'named' to avoid unnecessary configuration inside flutter.groovy
#157221
Conversation
|
FWIW , there is problem with |
|
FWIW , i highly recomand refactoring the usage of findByName into the same bloc so it became(if decided to not be removed): def tasksToCheck = [
"compress${variant.name.capitalize()}Assets",
"bundle${variant.name.capitalize()}Aar",
"bundle${variant.name.capitalize()}LocalLintAar"
]
tasksToCheck.each { taskName ->
def task = project.tasks.findByName(taskName)
if (task) {
task.dependsOn(copyFlutterAssetsTask)
}
}
|
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
reidbaker
left a comment
There was a problem hiding this comment.
test-exempt: Building aars are tested in integration tests and this specific warning is a groovy/gradle lint that we do not have enabled in ci.
| ] | ||
| tasksToCheck.each { taskTocheck -> | ||
| try { | ||
| project.tasks.named(taskTocheck).configure { task -> |
There was a problem hiding this comment.
This is literally the same execution code since before we would cause a configuration so from a safety standpoint I like it.
@gmackall what are your thoughts about removing the configure and just calling dependsOn since configuration isnt needed to do depends on according to this documentation.
There was a problem hiding this comment.
If we don't separately need the configuration here I support it.
I don't actually know if it is doing something important at this point, but if nothing breaks when removing it then that is fine with me
There was a problem hiding this comment.
@AbdeMohlbi do you have a preference between landing this pr as is and modifying the pr to no longer do the configuration, the downside to changing it now is that a revert reverts both parts. The downside to doing that is a new pr is that you will have to write another pr.
There was a problem hiding this comment.
if i remember correctly i tried deleting it and some tests broke , not sure tho i'm having headache from this file lately
|
i did an other pr that does the same thing with another isntance that i haven't seen at the start |
|
one more thing out of context of this pr is that where do i find an AndroidManifest.xml file of a flutter app that contains deep links , i'm trying to land this and IMO something is wrong , when trying to see what tags the script does found i got nothing ?? (i tried flutter deep links) |
Thank you and let's land this as is. |
…n` inside flutter.groovy (flutter/flutter#157221)
…n` inside flutter.groovy (flutter/flutter#157221)
…n` inside flutter.groovy (flutter/flutter#157221)
…n` inside flutter.groovy (flutter/flutter#157221)
…n` inside flutter.groovy (flutter/flutter#157221)
see #147122 for context
gradle docs for reference(scroll until this)
also this the lint :
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.