Repository navigation
feat(flutter_tools): add IntOptionDescriptor and DefaultedIntOptionDescriptor - #192455
Conversation
…scriptor Adds IntOptionDescriptor (OptionDescriptor<int?>) and DefaultedIntOptionDescriptor (OptionDescriptor<int>) to option_descriptor.dart. - Parses single-value integer CLI options and throws a descriptive FormatException on invalid numeric strings. - Migrates DebuggingOptionDescriptors.ddsPort, WebOptions.webPort, and WebOptions.webBrowserDebugPort from StringOptionDescriptor to IntOptionDescriptor. - Eliminates redundant manual int.tryParse / int.parse call sites in FlutterCommand.ddsPort and RunCommand. - Adds comprehensive unit tests in option_descriptor_test.dart.
…ults.getValue - Catches FormatException in SafeArgResults.getValue and calls throwToolExit(e.message) so invalid integer flags (--dds-port=abc, --web-port=abc) exit cleanly instead of triggering unhandled tool crash reports. - Deduplicates integer parsing logic into _IntOptionDescriptorBase._parseValue. - Removes redundant wasParsed(WebOptions.webBrowserDebugPort) check in RunCommandBase. - Adds unit test verifying SafeArgResults.getValue converts FormatException into ToolExit.
There was a problem hiding this comment.
Code Review
This pull request introduces IntOptionDescriptor and DefaultedIntOptionDescriptor to natively support integer command-line options, replacing manual string parsing for options such as dds-port, web-port, and web-browser-debug-port. Additionally, SafeArgResults.getValue is updated to catch FormatException and throw a ToolExit. The review feedback suggests restricting the defaultsTo parameter type to int? in the _IntOptionDescriptorBase constructor to ensure compile-time type safety.
bkonyi
left a comment
There was a problem hiding this comment.
LGTM other than the DDS port concern.
…default 0 - Updates DebuggingOptionDescriptors.ddsPort from IntOptionDescriptor to DefaultedIntOptionDescriptor with defaultsTo: 0. - Simplifies FlutterCommand.ddsPort to directly return getValue(DebuggingOptionDescriptors.ddsPort) when explicit ddsPort or fallback random port 0 is used.
|
autosubmit label was removed for flutter/flutter/192455, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…lutter#193323) Migrate `BuildLinuxCommand` (`build linux`) and `BuildWindowsCommand` (`build windows`) to typed `OptionDescriptor`s and `OptionBundle`s (following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, and flutter#192818): - Add `DesktopBuildOptionsBundle` in `common_options.dart`, encapsulating `CommonBuildOptionsBundle`, `BuildModeOptionsBundle`, `DartCompileOptionsBundle`, `BuildInfoOptions.flavor`, `splitDebugInfo`, `obfuscate`, `extraFrontEndOptions`, `extraGenSnapshotOptions`, `performanceMeasurementFile`, `analyzeSize`, `codeSizeDirectory`, `trackWidgetCreation`, and `configOnly`. - Migrate `BuildLinuxCommand` (`build_linux.dart`) to `DesktopBuildOptionsBundle`, `DefaultedEnumOptionDescriptor<TargetPlatform>` (`_targetPlatformOption`), and `DefaultedStringOptionDescriptor` (`_targetSysroot`), replacing untyped `boolArg('config-only')`, `stringArg('target-platform')!`, and `stringArg('target-sysroot')!` queries. - Migrate `BuildWindowsCommand` (`build_windows.dart`) to `DesktopBuildOptionsBundle` and `getValue(DesktopBuildOptionsBundle.configOnly)`. - Delete the obsolete `FlutterCommand.addCommonDesktopBuildOptions` helper method from `flutter_command.dart` (no remaining callers in `flutter_tools`). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…lutter#194017) ### Rationale `FlutterTestRunner.runTests` accepted 33 loose parameters (duplicating 23 of them on `runTestsBySpawningLightweightEngines` alongside duplicated `package:test` CLI argument lists), and `DriverService.startTest` accepted 12 loose parameters fed by untyped `boolArg` / `stringArg` / regex splitting in `TestCommand` and `DriveCommand`. Following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, flutter#192818, and flutter#193323, this change introduces immutable service-layer specification objects and migrates `TestCommand` and `DriveCommand` to typed `OptionDescriptor`s. ### Summary of Changes - Introduce `@immutable class TestFilterSpecification` and `@immutable class TestExecutionSpecification` (with `toPackageTestArgs()`) in `test/runner.dart`, shrinking `FlutterTestRunner.runTests` from 33 parameters to 3 (`testWrapper`, `testFiles`, `spec`) and `runTestsBySpawningLightweightEngines` from 25 parameters to 2 (`testFiles`, `spec`). - Migrate `TestCommand` (`commands/test.dart`) from inline `argParser` registrations and untyped `boolArg`/`stringArg`/`stringsArg` queries to private typed `OptionDescriptor` constants (`argParser.addDescriptors` + `getValue` / `wasParsed`), and update `bin/fuchsia_tester.dart`. - Introduce `@immutable class DriveTestSpecification` (with `parseBrowserDimension`) in `drive/drive_service.dart`, shrinking `DriverService.startTest` (and `FlutterDriverService` / `WebDriverService`) from 12 parameters to 2 (`testFile`, `spec`). - Migrate `DriveCommand` (`commands/drive.dart`) from inline registrations and ad-hoc string/int/enum parsing to private typed `OptionDescriptor` constants (`IntOptionDescriptor`, `DefaultedEnumOptionDescriptor<Browser>`, etc.) and `DriveTestSpecification`. - Update unit tests across `test_test.dart`, `drive_test.dart`, `drive_service_test.dart`, and `web_driver_service_test.dart`. ### Flow / Surface Delta ```mermaid flowchart LR A["TestCommand (OptionDescriptors)"] --> B["TestExecutionSpecification"] B --> C["FlutterTestRunner.runTests(spec)"] D["DriveCommand (OptionDescriptors)"] --> E["DriveTestSpecification"] E --> F["DriverService.startTest(spec)"] ``` ### Verification - Executed `dart analyze --fatal-infos` on `packages/flutter_tools` with 0 issues. - Executed `dart test` across `test_test.dart`, `drive_test.dart`, `drive_service_test.dart`, and `web_driver_service_test.dart` (`148/148` passing). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…fig to AndroidBuildInfo (flutter#194016) ### Rationale `BuildInfo` is the cross-platform build configuration passed across Android, iOS, Web, Desktop, and Fuchsia targets, yet it carried 6 loose Android/Gradle fields (`androidProjectArgs`, `androidGradleProjectCacheDir`, `androidGradleDaemon`, `androidSkipBuildDependencyValidation`, `androidEnableHcpp`, `explicitAndroidEnableHcpp`) and the Gradle `-P` flag serializer `toGradleConfig()`. Following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, flutter#192818, and flutter#193323, this change encapsulates those fields into `AndroidGradleConfig` and moves `toGradleConfig()` onto `AndroidBuildInfo` where all production callers already operate. ### Summary of Changes - Introduce `@immutable class AndroidGradleConfig` in `build_info.dart` and replace the 6 loose `android*` parameters on `BuildInfo` and `BuildInfo.copyWith` with `androidGradleConfig` (shrinking `BuildInfo`'s constructor from 28 to 23 parameters while preserving `AndroidDevice.startApp` forwarding during `flutter run`). - Move `toGradleConfig()` off `BuildInfo` onto `AndroidBuildInfo` (with zero `@deprecated` forwarders) and add `gradleDaemon` and `skipBuildDependencyValidation` convenience getters on `AndroidBuildInfo`. - Update `FlutterCommand.getBuildInfo()` in `flutter_command.dart` to construct `AndroidGradleConfig`. - Update `buildGradleApp`, `buildGradleAar`, and `_checkJavaAndGradleCompatibility` in `gradle.dart` to read Gradle configuration via `AndroidBuildInfo`. - Update unit tests across `build_info_test.dart`, `flutter_command_test.dart`, `android_gradle_builder_test.dart`, and `build_aar_test.dart`. ### Flow / Surface Delta ```mermaid flowchart LR A["FlutterCommand.getBuildInfo()"] --> B["BuildInfo(androidGradleConfig)"] B --> C["AndroidBuildInfo.toGradleConfig()"] C --> D["gradle.dart (-P flags)"] ``` ### Verification - Executed `dart analyze --fatal-infos` on `packages/flutter_tools` with 0 issues. - Executed `dart test` across `build_info_test.dart`, `flutter_command_test.dart`, `android_gradle_builder_test.dart`, and `build_aar_test.dart` (`160/160` passing). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…lutter#193323) Migrate `BuildLinuxCommand` (`build linux`) and `BuildWindowsCommand` (`build windows`) to typed `OptionDescriptor`s and `OptionBundle`s (following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, and flutter#192818): - Add `DesktopBuildOptionsBundle` in `common_options.dart`, encapsulating `CommonBuildOptionsBundle`, `BuildModeOptionsBundle`, `DartCompileOptionsBundle`, `BuildInfoOptions.flavor`, `splitDebugInfo`, `obfuscate`, `extraFrontEndOptions`, `extraGenSnapshotOptions`, `performanceMeasurementFile`, `analyzeSize`, `codeSizeDirectory`, `trackWidgetCreation`, and `configOnly`. - Migrate `BuildLinuxCommand` (`build_linux.dart`) to `DesktopBuildOptionsBundle`, `DefaultedEnumOptionDescriptor<TargetPlatform>` (`_targetPlatformOption`), and `DefaultedStringOptionDescriptor` (`_targetSysroot`), replacing untyped `boolArg('config-only')`, `stringArg('target-platform')!`, and `stringArg('target-sysroot')!` queries. - Migrate `BuildWindowsCommand` (`build_windows.dart`) to `DesktopBuildOptionsBundle` and `getValue(DesktopBuildOptionsBundle.configOnly)`. - Delete the obsolete `FlutterCommand.addCommonDesktopBuildOptions` helper method from `flutter_command.dart` (no remaining callers in `flutter_tools`). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…lutter#194017) ### Rationale `FlutterTestRunner.runTests` accepted 33 loose parameters (duplicating 23 of them on `runTestsBySpawningLightweightEngines` alongside duplicated `package:test` CLI argument lists), and `DriverService.startTest` accepted 12 loose parameters fed by untyped `boolArg` / `stringArg` / regex splitting in `TestCommand` and `DriveCommand`. Following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, flutter#192818, and flutter#193323, this change introduces immutable service-layer specification objects and migrates `TestCommand` and `DriveCommand` to typed `OptionDescriptor`s. ### Summary of Changes - Introduce `@immutable class TestFilterSpecification` and `@immutable class TestExecutionSpecification` (with `toPackageTestArgs()`) in `test/runner.dart`, shrinking `FlutterTestRunner.runTests` from 33 parameters to 3 (`testWrapper`, `testFiles`, `spec`) and `runTestsBySpawningLightweightEngines` from 25 parameters to 2 (`testFiles`, `spec`). - Migrate `TestCommand` (`commands/test.dart`) from inline `argParser` registrations and untyped `boolArg`/`stringArg`/`stringsArg` queries to private typed `OptionDescriptor` constants (`argParser.addDescriptors` + `getValue` / `wasParsed`), and update `bin/fuchsia_tester.dart`. - Introduce `@immutable class DriveTestSpecification` (with `parseBrowserDimension`) in `drive/drive_service.dart`, shrinking `DriverService.startTest` (and `FlutterDriverService` / `WebDriverService`) from 12 parameters to 2 (`testFile`, `spec`). - Migrate `DriveCommand` (`commands/drive.dart`) from inline registrations and ad-hoc string/int/enum parsing to private typed `OptionDescriptor` constants (`IntOptionDescriptor`, `DefaultedEnumOptionDescriptor<Browser>`, etc.) and `DriveTestSpecification`. - Update unit tests across `test_test.dart`, `drive_test.dart`, `drive_service_test.dart`, and `web_driver_service_test.dart`. ### Flow / Surface Delta ```mermaid flowchart LR A["TestCommand (OptionDescriptors)"] --> B["TestExecutionSpecification"] B --> C["FlutterTestRunner.runTests(spec)"] D["DriveCommand (OptionDescriptors)"] --> E["DriveTestSpecification"] E --> F["DriverService.startTest(spec)"] ``` ### Verification - Executed `dart analyze --fatal-infos` on `packages/flutter_tools` with 0 issues. - Executed `dart test` across `test_test.dart`, `drive_test.dart`, `drive_service_test.dart`, and `web_driver_service_test.dart` (`148/148` passing). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…fig to AndroidBuildInfo (flutter#194016) ### Rationale `BuildInfo` is the cross-platform build configuration passed across Android, iOS, Web, Desktop, and Fuchsia targets, yet it carried 6 loose Android/Gradle fields (`androidProjectArgs`, `androidGradleProjectCacheDir`, `androidGradleDaemon`, `androidSkipBuildDependencyValidation`, `androidEnableHcpp`, `explicitAndroidEnableHcpp`) and the Gradle `-P` flag serializer `toGradleConfig()`. Following flutter#191018, flutter#191510, flutter#191760, flutter#192307, flutter#192455, flutter#192524, flutter#192818, and flutter#193323, this change encapsulates those fields into `AndroidGradleConfig` and moves `toGradleConfig()` onto `AndroidBuildInfo` where all production callers already operate. ### Summary of Changes - Introduce `@immutable class AndroidGradleConfig` in `build_info.dart` and replace the 6 loose `android*` parameters on `BuildInfo` and `BuildInfo.copyWith` with `androidGradleConfig` (shrinking `BuildInfo`'s constructor from 28 to 23 parameters while preserving `AndroidDevice.startApp` forwarding during `flutter run`). - Move `toGradleConfig()` off `BuildInfo` onto `AndroidBuildInfo` (with zero `@deprecated` forwarders) and add `gradleDaemon` and `skipBuildDependencyValidation` convenience getters on `AndroidBuildInfo`. - Update `FlutterCommand.getBuildInfo()` in `flutter_command.dart` to construct `AndroidGradleConfig`. - Update `buildGradleApp`, `buildGradleAar`, and `_checkJavaAndGradleCompatibility` in `gradle.dart` to read Gradle configuration via `AndroidBuildInfo`. - Update unit tests across `build_info_test.dart`, `flutter_command_test.dart`, `android_gradle_builder_test.dart`, and `build_aar_test.dart`. ### Flow / Surface Delta ```mermaid flowchart LR A["FlutterCommand.getBuildInfo()"] --> B["BuildInfo(androidGradleConfig)"] B --> C["AndroidBuildInfo.toGradleConfig()"] C --> D["gradle.dart (-P flags)"] ``` ### Verification - Executed `dart analyze --fatal-infos` on `packages/flutter_tools` with 0 issues. - Executed `dart test` across `build_info_test.dart`, `flutter_command_test.dart`, `android_gradle_builder_test.dart`, and `build_aar_test.dart` (`160/160` passing). ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
Adds IntOptionDescriptor (OptionDescriptor<int?>) and DefaultedIntOptionDescriptor (OptionDescriptor) to option_descriptor.dart.