Create a FlutterPluginUtils.kt, and port static methods from FlutterPlugin there
#165239
Conversation
| // minimum version does support the replacement we can replace by changing a single line. | ||
| @JvmStatic | ||
| @Suppress("DEPRECATION") | ||
| internal fun capitalize(string: String): String = string.capitalize() |
There was a problem hiding this comment.
Open to removing this, I meant to actually make this the suggested replacement but we can't use the syntax because of our min version constraints.
| * A collection of static utility functions used by the Flutter Gradle Plugin. | ||
| */ | ||
| object FlutterPluginUtils { | ||
| // Gradle properties. |
There was a problem hiding this comment.
If these are used in other files can you add a pointer to those files? If they are used only by referencing constants can you say that in the kt doc?
There was a problem hiding this comment.
Hmm intellisense should work here for finding references, so I would prefer if we instead relied on that here (it even works cross groovy boundary). Does that make sense to you?
There was a problem hiding this comment.
We can talk about it in person but do think that a comment is correct. This is one of the issues with using strings. Yes tooling can find the same string in many places in code. The documentation defines where we intend the strings to match.
| * Kotlin (settings.gradle.kts). This is the same behavior as Gradle 8.5. | ||
| */ | ||
| @JvmStatic | ||
| internal fun settingsGradleFile(project: Project): File { |
There was a problem hiding this comment.
On this an other methods taking in a project is a large scope/object. Consider if you can reduce what is passed in to variables that are more restricted.
For example in this method I think you can pass project.projectDir and project.logger and still have full functionality.
I think a bunch of the methods that use properties will need to stay the same here are some others I think can have reduced input scope.
- buildGradleFile
- shouldConfigureFlutterTask
- pluginSupportsAndroidPlatform
There was a problem hiding this comment.
Minimized the scope of buildGradleFile and settingsGradleFile, and changed the names.
I haven't minimized the scope of the others, because I feel like it starts to leak some of the implementation out of the methods themselves
e.g. shouldConfigureFlutterTask could take a taskNames: List<String> instead of a project, but I'm not sure it's immediately obvious to a caller that you get that list with project.gradle.startParameter.taskNames, or that whether a plugin project supports Android is a function of it's projectDir.
But can reconsider if you feel strongly that those methods should also have their scope minimized
There was a problem hiding this comment.
If there is a good reason to take a project that is fine with me. Thank you for taking a pass to see if we could reduce scope.
|
autosubmit label was removed for flutter/flutter/165239, because - The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label. |
| // Gradle properties. | ||
| internal const val PROP_SHOULD_SHRINK_RESOURCES = "shrink" | ||
| internal const val PROP_SPLIT_PER_ABI = "split-per-abi" | ||
| internal const val PROP_LOCAL_ENGINE_REPO = "localEngineRepo" |
There was a problem hiding this comment.
This should be "local-engine-repo"
There was a problem hiding this comment.
Wow I'm surprised we didn't have a test that would catch this. Will revert and reland
There was a problem hiding this comment.
Ok looks like the autorevert is gonna be difficult due to merge conflicts, will just approve the fix forward
|
Reason for revert: #165239 (comment) |
|
Unable to create the revert pull request due to ProcessException: Standard out |
Creates a
FlutterPluginUtils.ktobject to contain static methods used by the Flutter Gradle plugin, and moves over (porting them from Groovy to Kotlin)FlutterPlugin.projectmember. These methods were made static by changing their signature to take the project as an argument.This doesn't get all of the methods of type 2. Specifically, I'm planning on leaving functions that interact with the Android Gradle plugin for a different PR, as this PR was getting long enough and those pieces will be a lot more brittle anyways (so I'd like to be able to revert them on their own).
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.