Repository navigation
[flutter_tools] migrate getBuildInfo() to typed OptionDescriptors and BuildInfoOptionsBundle - #191510
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors 'FlutterCommand' to retrieve command-line arguments using typed option descriptors ('BuildInfoOptions', 'CommonOptions', and 'WebOptions') via 'getValue', replacing direct 'argParser' lookups. It also introduces the 'BuildInfoOptions' class and its corresponding option bundle, along with unit tests to verify default values and historical aliases. Feedback suggests refactoring a redundant 'getValue' call for the code size directory, using literal strings instead of confusing constants for front-end and gen-snapshot aliases, and adding documentation comments to the newly introduced public members in 'BuildInfoOptions' to comply with the style guide.
There was a problem hiding this comment.
Code Review
This pull request refactors FlutterCommand to retrieve command-line options using typed option descriptors via getValue instead of directly querying argParser.options. It introduces BuildInfoOptions and BuildInfoOptionsBundle to centralize these descriptors, along with corresponding unit tests. The review feedback suggests removing redundant .toList() calls on getValue results and adding documentation comments to the new public static fields in BuildInfoOptions to comply with the style guide.
…ter#191760) Third step in the type-safe, declarative CLI argument refactor for `flutter_tools` (following flutter#191018 and flutter#191510). This PR introduces typed enum option descriptors and defaulted string/enum descriptors, and migrates existing Web and Common options to use them. ### Key Changes 1. **`EnumOptionDescriptor<T extends Enum>` & `DefaultedEnumOptionDescriptor<T extends Enum>` (`lib/src/runner/options/option_descriptor.dart`)**: - Automatically derives `allowed` values via `values.map(...)`. - Supports `nameMapper`, `valueParser`, and automatic `CliEnum` mapping for `allowed` and `allowedHelp`. - Returns strongly-typed `T?` or `T` from `getValue()`, eliminating manual `fromCliName()` parsing and casting. 2. **`DefaultedStringOptionDescriptor` (`lib/src/runner/options/option_descriptor.dart`)**: - Requires concrete non-null `defaultsTo`. - Statically types `getValue()` to return non-nullable `String`. - Removes ad-hoc `getValueOrDefault()` helper from `StringOptionDescriptor`. 3. **Migration of Existing Options**: - Migrated `WebOptions.pwaStrategy` from `static final StringOptionDescriptor` to `const EnumOptionDescriptor<ServiceWorkerStrategy>`. - Migrated `CommonOptions.target` from `StringOptionDescriptor` to `const DefaultedStringOptionDescriptor`. - Updated `build_web.dart` to pass `getValue(WebOptions.pwaStrategy)` directly without manual `fromCliName()` conversion. ### Verification - Whole-package `dart analyze` across `packages/flutter_tools`: **0 issues**. - Unit and command tests (`option_descriptor_test.dart`, `build_info_options_bundle_test.dart`, `build_web_test.dart`): **55/55 tests passing**.
…criptor (flutter#192307) Migrates `createDebuggingOptions()` in `RunCommandBase` (`lib/src/commands/run.dart`) and associated option registration helpers in `FlutterCommand` (`lib/src/runner/flutter_command.dart`) to typed `OptionDescriptor` instances, continuing the CLI argument architecture modernization started in flutter#191018, flutter#191510, and flutter#191760. ## Summary of Changes * **Typed Debugging Option Descriptors (`DebuggingOptionDescriptors` in `common_options.dart`)**: * Defined 35+ strongly-typed descriptors covering engine flags (`enableImpeller`, `enableFlutterGpu`, `enableVulkanValidation`, `enableEmbedderApi`, `enableHcpp`), tracing/profiling (`traceStartup`, `traceSystrace`, `traceToFile`, `endlessTraceBuffer`, `profileMicrotasks`, `traceSkia`, `traceAllowlist`, `traceSkiaAllowlist`, `enableDartProfiling`, `profileStartup`, `cacheStartupProfile`, `verboseSystemLogs`, `purgePersistentCache`), VM service/DevTools/DDS (`dds`, `ddsPort`, `disableDds`, `enableDevTools`, `devToolsServerAddress`, `vmserviceOutFile`, `disableServiceAuthCodes`, `disableServiceOriginCheck`, `startPaused`, `dartFlags`, `ipv6`), and runner configuration (`route`, `enableSoftwareRendering`, `skiaDeterministicRendering`, `dartEntrypointArgs`, `uninstallFirst`, `iosProfileDebugger`, `useTestFonts`, `adbLogFiltering`, `testFlag`). * **Web Server & Debugging Descriptors (`WebOptions` in `web_options.dart`)**: * Added typed descriptors for `webHeader`, `webHostname`, `webPort`, `webTlsCertPath`, `webTlsCertKeyPath`, `webServerDebugProtocol`, `webServerDebugBackendProtocol`, `webServerDebugInjectedClientProtocol`, `webAllowExposeUrl`, `webRunHeadless`, `webBrowserDebugPort`, `webEnableExpressionEvaluation`, `webLaunchUrl`, `webBrowserFlags`, and `crossOriginIsolation`. * **`ArgParserDescriptorExtension` & Automatic `Expando` Registry (`option_descriptor.dart`)**: * Added `addDescriptor` / `addDescriptors` extension methods on `ArgParser` for concise option registration. * Replaced explicit `Map<String, OptionDescriptor>? registry` parameter plumbing across `OptionDescriptor.addTo()`, `OptionBundle.register()`, and `FlutterCommand._optionRegistry` with an internal `Expando<Map<String, OptionDescriptor<Object?>>>` on `ArgParser`, ensuring automatic diamond-bundle de-duplication and conflict detection across all registration paths. * **Tri-State `bool?` Semantics**: * Used `NullableFlagOptionDescriptor` (`defaultsTo: null`) for flags whose omitted state is semantically distinct from `false` (`enable-impeller`, `enable-flutter-gpu`, `enable-hcpp`, `cross-origin-isolation`, `ios-profile-debugger`), allowing `getValue(...)` to preserve `bool?` without manual string lookups. * **Eliminated Untyped Lookups in `createDebuggingOptions()`**: * Replaced 35+ untyped `argParser.options.containsKey('...')`, `boolArg('...')`, `stringArg('...')`, and `stringsArg('...')` lookups in `RunCommandBase.createDebuggingOptions()`, `webDevServerConfigCore()`, and `FlutterCommand` helpers (`usesWebOptions`, `addDdsOptions`, `addDevToolsOptions`, `explicitEnableHcpp`, `ddsPort`, `devToolsServerAddress`) with type-safe `getValue(...)`, `wasParsed(...)`, and `hasOption(...)` calls. ## 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]. - [x] 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] All existing and new tests are passing.
Second step in the type-safe, declarative CLI argument refactor for
flutter_tools(following flutter/flutter#191018).This PR migrates
getBuildInfo()inFlutterCommandto use typed option descriptors andSafeArgResults, eliminating 21 defensive string-basedargParser.options.containsKey('...')queries.Key Changes
BuildInfoOptionsBundle(lib/src/runner/options/common_options.dart):BuildInfocreation (track-widget-creation,analyze-size,code-size-directory,obfuscate,split-debug-info,android-gradle-daemon,android-project-arg,android-project-cache-dir,android-skip-build-dependency-validation,performance-measurement-file,flavor,codesign,frontend-server-starter-path,initialize-from-dill,assume-initialize-from-dill-up-to-date,extra-front-end-options,extra-gen-snapshot-options).constoption descriptors maintaining 100% backward compatibility for names, abbreviations, aliases, help strings, and defaults.getBuildInfo()Refactor (lib/src/runner/flutter_command.dart):wasParsed()/getValue()calls viaSafeArgResults.Testing & Verification:
test/general.shard/runner/options/build_info_options_bundle_test.dartverifying all descriptors register properly.dart analyzeacrosspackages/flutter_tools: 0 issues.flutter_command_test.dart,build_test.dart): 250/250 passing.