Repository navigation
[tool] Refactor artifacts.dart to use enhanced enums and reduce duplication - #187063
Merged
auto-submit[bot] merged 6 commits intoMay 26, 2026
Merged
auto-submit[bot] merged 6 commits into
auto-submit[bot] merged 6 commits into
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. |
github-actions
Bot
added
the
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Artifact and HostArtifact enums in packages/flutter_tools/lib/src/artifacts.dart to store their associated file names and metadata directly within the enum definitions. This allows removing the large, centralized helper functions _artifactToFileName and _hostArtifactToFileName in favor of calling getFileName directly on the enum instances. A review comment suggests refactoring getFileName to throw errors instead of returning null or using assert for invalid states, which would allow changing the return type from String? to String and removing numerous trailing ! null assertions at call sites.
Comment on lines
+108
to
+121
| String? getFileName(Platform hostPlatform, [BuildMode? mode]) { | ||
| if (isPatchedSdk) { | ||
| assert(false, 'No filename for sdk path, should not be invoked'); | ||
| return null; | ||
| } | ||
| if (isFuchsiaRunner) { | ||
| assert(mode != null, 'BuildMode is required for fuchsiaFlutterRunner'); | ||
| final jitOrAot = mode!.isJit ? '_jit' : '_aot'; | ||
| final productOrNo = mode.isRelease ? '_product' : ''; | ||
| return 'flutter$jitOrAot${productOrNo}_runner-0.far'; | ||
| } | ||
| final exe = (isExecutable && hostPlatform.isWindows) ? '.exe' : ''; | ||
| return '$_fileName$exe'; | ||
| } |
Contributor
There was a problem hiding this comment.
By throwing a StateError instead of returning null when isPatchedSdk is true, and throwing an ArgumentError instead of using assert when mode is null for isFuchsiaRunner, we can change the return type of getFileName from String? to String.
This makes the API much safer (since assert is ignored in release builds, which could lead to unexpected null pointer exceptions later) and allows us to remove all the trailing ! null assertion operators at the call sites throughout this file (e.g., lines 565, 622, 625, 628, 770, 858, 896, 914, 931, 963, 980, 1066, 1210, 1227).
This simplification aligns with the repository style guide's principles of optimizing for readability and suggesting refactoring to enhance maintainability.
String getFileName(Platform hostPlatform, [BuildMode? mode]) {
if (isPatchedSdk) {
throw StateError('No filename for sdk path, should not be invoked');
}
if (isFuchsiaRunner) {
if (mode == null) {
throw ArgumentError('BuildMode is required for fuchsiaFlutterRunner');
}
final jitOrAot = mode.isJit ? '_jit' : '_aot';
final productOrNo = mode.isRelease ? '_product' : '';
return 'flutter$jitOrAot${productOrNo}_runner-0.far';
}
final exe = (isExecutable && hostPlatform.isWindows) ? '.exe' : '';
return '$_fileName$exe';
}References
- Suggest simplification and refactoring: Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
github-actions
Bot
added
platform-windows
|
This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled. |
…cation Refactors `Artifact` and `HostArtifact` to enhanced enums, encapsulating filename mapping logic and introducing a `.directory()` constructor for directory-representing values. Also simplifies `getHostArtifact` implementations across different `Artifacts` classes by delegating non-engine artifacts and consolidating web SDK artifact resolution to a shared helper.
Addresses review comment from gemini-code-assist: - Refactors `Artifact.getFileName` to throw `StateError` or `ArgumentError` instead of returning `null` or using `assert`. - Changes the return type of `Artifact.getFileName` from `String?` to non-nullable `String`. - Removes all trailing null assertions (`!`) at call sites of `getFileName`.
Refactors the HostPlatform enum in packages/flutter_tools/lib/src/base/os.dart to an enhanced enum. Stores cliName and platformName in the constructor, removing getNameForHostPlatform. TAG=agy CONV=f3b1231c-cf85-4cdb-a44a-6dbc7369d139
…e getters TAG=agy CONV=f3b1231c-cf85-4cdb-a44a-6dbc7369d139
bkonyi
force-pushed
the
refactor-artifacts-enhanced-enums-20260525
branch
from
May 26, 2026 14:31
aa456ce to
eabc55b
Compare
…rgetPlatform Restores `getNameForHostPlatform`, `getNameForTargetPlatform`, and `getTargetPlatformForName` as deprecated compatibility shims. This prevents breaking internal Google tools or other plugins that might be importing these private helper APIs, while keeping the main enum designs clean. Also formats the conflicted test files.
- Add flutter_ignore comment to deprecation shims to bypass dev/bots/analyze.dart custom syntax checks. - Fix CWD check in localizations_utils.dart to support Git worktrees (.git file check). - Format all modified test files to comply with formatting rules.
chingjun
approved these changes
May 26, 2026
flutter-dashboard
Bot
removed
the
autosubmit
auto-submit Bot
pushed a commit
to flutter/packages
that referenced
this pull request
May 27, 2026
flutter/flutter@f3a4b98...c8f2f16 2026-05-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Flutter GPU] Add explicit draw counts (#186639)" (flutter/flutter#187170) 2026-05-27 [email protected] Roll Dart SDK from 7dcea971af6b to f3db7b7d9801 (2 revisions) (flutter/flutter#187144) 2026-05-27 [email protected] [Flutter GPU] Add explicit draw counts (flutter/flutter#186639) 2026-05-27 [email protected] Roll Skia from f9db7748563e to fa944af10f91 (1 revision) (flutter/flutter#187139) 2026-05-27 [email protected] [Flutter GPU] Flutter GPU ShaderLibrary in-place reload for shader hot reload (flutter/flutter#186793) 2026-05-27 [email protected] Roll Skia from a692cbf38952 to f9db7748563e (8 revisions) (flutter/flutter#187135) 2026-05-27 [email protected] Use local semantics tester in Material button tests (flutter/flutter#186667) 2026-05-27 [email protected] Filter out `a: text input` from `team-framework` PR triage (flutter/flutter#187129) 2026-05-27 [email protected] [Engine] Fix silent buffer mismatch bug in FML Win32 CommandLineFromPlatform (flutter/flutter#187120) 2026-05-27 [email protected] examples: Remove unused parameters (flutter/flutter#185819) 2026-05-27 [email protected] Remove another `String.valueOf` (flutter/flutter#186628) 2026-05-27 [email protected] Print trace when skipping flavor-specific and platform-specific assets (flutter/flutter#187045) 2026-05-26 [email protected] Fix `InteractiveViewer` memory leak from undisposed `CurvedAnimation`s (flutter/flutter#185826) 2026-05-26 [email protected] Fix AccessibilityBridge startup crash on vendor-modified Android (flutter/flutter#184630) 2026-05-26 [email protected] Update data driven fixes docs (flutter/flutter#186217) 2026-05-26 [email protected] Add more 0x0 size tests part10 (flutter/flutter#186201) 2026-05-26 [email protected] Disable spell check for obscured text (flutter/flutter#186329) 2026-05-26 [email protected] [iOS] Migrate VSyncClient and DisplayLinkManager to Swift (flutter/flutter#187001) 2026-05-26 [email protected] [flutter_tools, devicelab] Add safety filesystem guard to tests (flutter/flutter#186748) 2026-05-26 [email protected] Roll Fuchsia Linux SDK from Itd2Jq_ZIABH2rW7B... to k9EizfEGJO4WwQN9-... (flutter/flutter#187115) 2026-05-26 [email protected] Roll Dart SDK from 00e625453c43 to 7dcea971af6b (1 revision) (flutter/flutter#187117) 2026-05-26 [email protected] Remove use of deprecated copy_trees. (flutter/flutter#187091) 2026-05-26 [email protected] Use resolved implementation plugins with SwiftPM (flutter/flutter#187097) 2026-05-26 [email protected] Roll Skia from 27a819894f7c to a692cbf38952 (3 revisions) (flutter/flutter#187109) 2026-05-26 [email protected] Roll Packages from 69cf959 to fc795e5 (4 revisions) (flutter/flutter#187114) 2026-05-26 [email protected] Handle simctl process exceptions during discovery (flutter/flutter#186501) 2026-05-26 [email protected] Clarify precache enabled platforms help (flutter/flutter#186878) 2026-05-26 [email protected] [tool] Refactor artifacts.dart to use enhanced enums and reduce duplication (flutter/flutter#187063) 2026-05-26 [email protected] Build script updates for syncing engine sources and building artifacts on linux-arm64 (flutter/flutter#186917) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
10 tasks done
creatorpiyush
pushed a commit
to creatorpiyush/packages
that referenced
this pull request
Jun 10, 2026
…r#11795) flutter/flutter@f3a4b98...c8f2f16 2026-05-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Flutter GPU] Add explicit draw counts (#186639)" (flutter/flutter#187170) 2026-05-27 [email protected] Roll Dart SDK from 7dcea971af6b to f3db7b7d9801 (2 revisions) (flutter/flutter#187144) 2026-05-27 [email protected] [Flutter GPU] Add explicit draw counts (flutter/flutter#186639) 2026-05-27 [email protected] Roll Skia from f9db7748563e to fa944af10f91 (1 revision) (flutter/flutter#187139) 2026-05-27 [email protected] [Flutter GPU] Flutter GPU ShaderLibrary in-place reload for shader hot reload (flutter/flutter#186793) 2026-05-27 [email protected] Roll Skia from a692cbf38952 to f9db7748563e (8 revisions) (flutter/flutter#187135) 2026-05-27 [email protected] Use local semantics tester in Material button tests (flutter/flutter#186667) 2026-05-27 [email protected] Filter out `a: text input` from `team-framework` PR triage (flutter/flutter#187129) 2026-05-27 [email protected] [Engine] Fix silent buffer mismatch bug in FML Win32 CommandLineFromPlatform (flutter/flutter#187120) 2026-05-27 [email protected] examples: Remove unused parameters (flutter/flutter#185819) 2026-05-27 [email protected] Remove another `String.valueOf` (flutter/flutter#186628) 2026-05-27 [email protected] Print trace when skipping flavor-specific and platform-specific assets (flutter/flutter#187045) 2026-05-26 [email protected] Fix `InteractiveViewer` memory leak from undisposed `CurvedAnimation`s (flutter/flutter#185826) 2026-05-26 [email protected] Fix AccessibilityBridge startup crash on vendor-modified Android (flutter/flutter#184630) 2026-05-26 [email protected] Update data driven fixes docs (flutter/flutter#186217) 2026-05-26 [email protected] Add more 0x0 size tests part10 (flutter/flutter#186201) 2026-05-26 [email protected] Disable spell check for obscured text (flutter/flutter#186329) 2026-05-26 [email protected] [iOS] Migrate VSyncClient and DisplayLinkManager to Swift (flutter/flutter#187001) 2026-05-26 [email protected] [flutter_tools, devicelab] Add safety filesystem guard to tests (flutter/flutter#186748) 2026-05-26 [email protected] Roll Fuchsia Linux SDK from Itd2Jq_ZIABH2rW7B... to k9EizfEGJO4WwQN9-... (flutter/flutter#187115) 2026-05-26 [email protected] Roll Dart SDK from 00e625453c43 to 7dcea971af6b (1 revision) (flutter/flutter#187117) 2026-05-26 [email protected] Remove use of deprecated copy_trees. (flutter/flutter#187091) 2026-05-26 [email protected] Use resolved implementation plugins with SwiftPM (flutter/flutter#187097) 2026-05-26 [email protected] Roll Skia from 27a819894f7c to a692cbf38952 (3 revisions) (flutter/flutter#187109) 2026-05-26 [email protected] Roll Packages from 69cf959 to fc795e5 (4 revisions) (flutter/flutter#187114) 2026-05-26 [email protected] Handle simctl process exceptions during discovery (flutter/flutter#186501) 2026-05-26 [email protected] Clarify precache enabled platforms help (flutter/flutter#186878) 2026-05-26 [email protected] [tool] Refactor artifacts.dart to use enhanced enums and reduce duplication (flutter/flutter#187063) 2026-05-26 [email protected] Build script updates for syncing engine sources and building artifacts on linux-arm64 (flutter/flutter#186917) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
via-guy
pushed a commit
to via-guy/flutter
that referenced
this pull request
Jun 26, 2026
…cation (flutter#187063) Refactors `Artifact` and `HostArtifact` to enhanced enums, encapsulating filename mapping logic and introducing a `.directory()` constructor for directory-representing values. Also simplifies `getHostArtifact` implementations across different `Artifacts` classes by delegating non-engine artifacts and consolidating web SDK artifact resolution to a shared helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactors
ArtifactandHostArtifactto enhanced enums, encapsulating filename mapping logic and introducing a.directory()constructor for directory-representing values.Also simplifies
getHostArtifactimplementations across differentArtifactsclasses by delegating non-engine artifacts and consolidating web SDK artifact resolution to a shared helper.