Does the copy task actualy do anything #173564
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. |
There was a problem hiding this comment.
Code Review
This pull request is an experiment to determine if the copyFlutterAssets Gradle task is still necessary by commenting out most of its logic. My feedback focuses on suggesting the complete removal of the commented-out code instead of leaving it in the codebase, as version control can be used to revert the changes if the experiment fails. This approach helps maintain code clarity and adheres to the project's style guide principle of optimizing for readability.
| // dependsOn(compileTask) | ||
| // with(compileTask.assets) | ||
| filePermissions { | ||
| user { | ||
| read = true | ||
| write = true | ||
| } | ||
| } | ||
| if (isUsedAsSubproject) { | ||
| // TODO(gmackall): above is always false, can delete | ||
| dependsOn(packageAssets) | ||
| dependsOn(cleanPackageAssets) | ||
| into(packageAssets!!.outputs) | ||
| } | ||
| val mergeAssets = | ||
| try { | ||
| variant.mergeAssetsProvider.get() | ||
| } catch (e: IllegalStateException) { | ||
| // TODO(gmackall): Migrate to AGPs variant api. | ||
| // https://github.com/flutter/flutter/issues/166550 | ||
| @Suppress("DEPRECATION") | ||
| variant.mergeAssets | ||
| } | ||
| dependsOn(mergeAssets) | ||
| dependsOn("clean${FlutterPluginUtils.capitalize(mergeAssets.name)}") | ||
| mergeAssets.mustRunAfter("clean${FlutterPluginUtils.capitalize(mergeAssets.name)}") | ||
| into(mergeAssets.outputDir) | ||
| // if (isUsedAsSubproject) { | ||
| // // TODO(gmackall): above is always false, can delete | ||
| // dependsOn(packageAssets) | ||
| // dependsOn(cleanPackageAssets) | ||
| // into(packageAssets!!.outputs) | ||
| // } | ||
| // val mergeAssets = | ||
| // try { | ||
| // variant.mergeAssetsProvider.get() | ||
| // } catch (e: IllegalStateException) { | ||
| // // TODO(gmackall): Migrate to AGPs variant api. | ||
| // // https://github.com/flutter/flutter/issues/166550 | ||
| // @Suppress("DEPRECATION") | ||
| // variant.mergeAssets | ||
| // } | ||
| // dependsOn(mergeAssets) | ||
| // dependsOn("clean${FlutterPluginUtils.capitalize(mergeAssets.name)}") | ||
| // mergeAssets.mustRunAfter("clean${FlutterPluginUtils.capitalize(mergeAssets.name)}") | ||
| // into(mergeAssets.outputDir) |
There was a problem hiding this comment.
Instead of commenting out this large block of code, it would be better to delete it entirely. The purpose of this PR is to test if this code is necessary, and version control systems like Git are excellent for managing experiments like this. If it turns out this code is still needed, it can be easily restored by reverting the commit.
Leaving commented-out code can lead to confusion and code rot. By deleting it, you make the intent clearer: this code is believed to be unused. This also aligns with the style guide's principle of optimizing for readability.1 If the experiment is successful, the code is already gone. If not, it's a simple git revert away.
filePermissions {
user {
read = true
write = true
}
}
Reading the copyFlutterAssets gradle task there is a lot of configuration but surprisingly little copying of flutter assets.
This pr deletes most of what the task does in an effort to determine if the task is useful and/or if we have integration tests that cover flutter assets inclusion.
Related to #166550
From: packages/devicelab/
../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t module_host_with_custom_build_testPre-launch Checklist
///).