Repository navigation
Use Gradle KTS in new Android app projects by default - #154061
Conversation
Not until #154125 gets merged to master and we wait a few days to get some confidence that it's not going to get reverted. |
| static final List<RegExp> _declarativeKotlinPluginPatterns = <RegExp>[ | ||
| RegExp('^\\s*id\\s*\\(?\\s*[\'"]kotlin-android[\'"]\\s*\\)?\\s*\$'), | ||
| RegExp('^\\s*id\\s*\\(?\\s*[\'"]org.jetbrains.kotlin.android[\'"]\\s*\\)?\\s*\$'), | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Here's a small DartPad playground I made to ensure I got the regexes right.
There was a problem hiding this comment.
Please include some examples in a dart doc above this regex. I know we do not have them for other examples but I find it super helpful when reading the code.
|
No idea why |
|
out of band comms, this is ready for review |
| static final List<RegExp> _declarativeKotlinPluginPatterns = <RegExp>[ | ||
| RegExp('^\\s*id\\s*\\(?\\s*[\'"]kotlin-android[\'"]\\s*\\)?\\s*\$'), | ||
| RegExp('^\\s*id\\s*\\(?\\s*[\'"]org.jetbrains.kotlin.android[\'"]\\s*\\)?\\s*\$'), | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Please include some examples in a dart doc above this regex. I know we do not have them for other examples but I find it super helpful when reading the code.
|
|
||
| kotlinOptions { | ||
| jvmTarget = JavaVersion.VERSION_1_8 | ||
| jvmTarget = "1.8" |
There was a problem hiding this comment.
Out of curiosity why not use the constant and move to a string. We usually want to move away from string based types.
I ask because this value will get moved to java 11 shortly.
|
@bartekpacia just following up on this. Do you think you'll have time to address @reidbaker's comments? If not, this is probably something we can push over the finish line since it's a change we'd like to land. |
|
hey @bkonyi - sure I'm happy to do it! i was kinda distracted recently, sorry |
…view_sdk_test.dart`
…art` flutter_tools will now correctly detect if the Android app uses Kotlin in more situations.
…mismatch_test.dart`
|
done! also rebased with master |
|
|
||
| kotlinOptions { | ||
| jvmTarget = JavaVersion.VERSION_1_8 | ||
| jvmTarget = "1.8" |
There was a problem hiding this comment.
Should this also be JavaVersion.VERSION_1_8.toString(), like the other instance? I'd also like to avoid hardcoded strings here
There was a problem hiding this comment.
Yup you're right. I missed it. I'll fix it tomorrow.
Feel free to push to my branch though and merge it.
| String get rootPath => path.join(parent.path, name); | ||
| String get androidPath => path.join(rootPath, 'android'); | ||
| String get iosPath => path.join(rootPath, 'ios'); | ||
| File get appBuildFile { |
There was a problem hiding this comment.
Interesting that we have an independent FlutterProject here
(independent of
I don't think I was aware of this
There was a problem hiding this comment.
Do you think it's worth to create an issue to track this?
(I'm not sure)
There was a problem hiding this comment.
I'll take a look to try to understand why it exists and make an issue if it doesn't seem to me like it should
|
status: of course there were more tests that broke. Fixing them now. |
|
auto label is removed for flutter/flutter/154061, due to - The status or check suite Mac dart_plugin_registry_test has failed. Please fix the issues identified (or deflake) before re-applying this label. |
re-running fixed it |
|
Reason for revert: might be the reason that cause Framework tree to red |
This reverts commit d3c54a1.
|
Oof, yes this test definition will need changing Perhaps these two tests Also could have been avoided by #153399 |
|
@gmackall I'm on it |
Will attempt to do this in #157196 |
|
good idea, thanks! much appreciated |
…ter#157195) This PR resolves flutter#151166 This PR relands flutter#154061
This PR resolves #151166
Pre-launch Checklist
///).