-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[go_router_builder] Ignore upcoming experimental_member_use
warnings.
#10082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[go_router_builder] Ignore upcoming experimental_member_use
warnings.
#10082
Conversation
In an upcoming Dart SDK change (https://dart-review.googlesource.com/c/sdk/+/450970), I intend to add logic to the analyzer for generating a warning if an API marked `@experimental` is used. This will allow experimental analyzer features to be developed without creating a risk of breaking changes downstream. Unfortunately, the `go_router_builder` package uses an old version of the analyzer in which some parts of the API were marked as `@experimental`, even though they were fully supported. So to avoid breaking trybots when the new warning rolls out, I need to add `// ignore` comments to ignore the upcoming warnings. In a future PR, I intend to bump the `go_router_builder` package's analyzer dependency to 8.2.0; in that analyzer version, the erroneous `@experimental` annotations have been removed. Unfortunately, that can't be done yet, since `go_router_builder` still needs to support versions of flutter that pin analyzer to an earlier version. (See flutter#10079, which attempts to bump the analyzer dependency, but breaks trybots).
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds // ignore: experimental_member_use
comments across the go_router_builder
package. This is a proactive measure to avoid build failures from an upcoming Dart SDK change that will introduce warnings for using APIs marked as @experimental
. The changes are straightforward and well-explained. I have one suggestion regarding code formatting to ensure consistency with the project's style guide.
final FragmentDeclarationResult? declaration = parsedLibrary | ||
.getFragmentDeclaration(element.firstFragment); | ||
// ignore: experimental_member_use | ||
.getFragmentDeclaration(element.firstFragment); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting appears to have been missed by the auto-formatter. According to the Dart style guide, which this repository follows, code should be consistently formatted for readability.1 I'd suggest reformatting this assignment to improve its structure.
final FragmentDeclarationResult? declaration = parsedLibrary | |
.getFragmentDeclaration(element.firstFragment); | |
// ignore: experimental_member_use | |
.getFragmentDeclaration(element.firstFragment); | |
final FragmentDeclarationResult? declaration = | |
// ignore: experimental_member_use | |
parsedLibrary.getFragmentDeclaration(element.firstFragment); |
Style Guide References
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll change it. But next time the auto-formatter is run it will change it back, so I think the benefit is minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like manually formatting this file caused a check to complain (https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8702634207105858353/+/u/Run_package_tests/format/stdout), so I'm switching back to the auto-formatted version.
@chunhtai do you think this PR needs a version change and/or changelog entry? My feeling is that it doesn't, because it's needed solely to avoid bot breakages. But I'd be happy to add them if you think I should. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except the gemini comment
override reason: lint only change, doesn't affect functionality and documentation |
e0f9276
to
8547070
Compare
test-exempt: comment-only change (I assume there was a version of this PR that didn't only touch comment lines, and that's what triggered the bot comment?) |
Yes. One of the comments I had to add caused the formatter to change the indentation of a non-comment line. I tried to manually adjust the formatting (as suggested in #10082 (comment)), but that caused some checks to fail that verify that the code has been property formatted (https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8702634207105858353/+/u/Run_package_tests/format/stdout), so I need to go back to the version that changes the indentation. |
8547070
to
e0f9276
Compare
flutter/packages@389c678...34eec78 2025-09-29 blackorbs.dev@gmail.com [camera_android_camerax] Implement setDescriptionWhileRecording (flutter/packages#10030) 2025-09-29 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump org.jetbrains.kotlin:kotlin-bom from 2.0.21 to 2.2.20 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#10114) 2025-09-27 stuartmorgan@google.com [various] Standardize Java compat in Gradle (flutter/packages#10091) 2025-09-26 stuartmorgan@google.com [various] Standardize gradle dependencies (flutter/packages#10083) 2025-09-26 paulberry@google.com [go_router_builder] Ignore upcoming `experimental_member_use` warnings. (flutter/packages#10082) 2025-09-26 stuartmorgan@google.com [various] Standardize Gradle `namespace` (flutter/packages#10084) 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-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
In an upcoming Dart SDK change
(https://dart-review.googlesource.com/c/sdk/+/450970), I intend to add logic to the analyzer for generating a warning if an API marked
@experimental
is used. This will allow experimental analyzer features to be developed without creating a risk of breaking changes downstream.Unfortunately, the
go_router_builder
package uses an old version of the analyzer in which some parts of the API were marked as@experimental
, even though they were fully supported. So to avoid breaking trybots when the new warning rolls out, I need to add// ignore
comments to ignore the upcoming warnings.In a future PR, I intend to bump the
go_router_builder
package's analyzer dependency to 8.2.0; in that analyzer version, the erroneous@experimental
annotations have been removed. Unfortunately, that can't be done yet, sincego_router_builder
still needs to support versions of flutter that pin analyzer to an earlier version. (See #10079, which attempts to bump the analyzer dependency, but breaks trybots).Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3