-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Replace [FinderBase] with [Finder] in the documentation of Matchers #168279
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
Conversation
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.
Isn't it more accurate to say FinderBase instead of Finder? Something like findsNothing could be used with SemanticsFinder, which is a FinderBase and not a Finder.
Edit: In your example in the issue #168230, you compare findsNothing with findsOneWidget, but findsOneWidget should operate only on Finders, so I think that is correct as-is.
|
I cannot understand you. Both FindsNothing and FindsOneWidget are instances of the same class: _FindsCountMatcher, so if something should work with one, it should work with the other, and if something should not work with one, it should not work with the other. |
|
Yes please go ahead and update the PR and I will take another look. Thank you! |
|
I did replace [Finder] with [FinderBase] in the doc comments of Matchers. |
justinmc
left a comment
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.
I think these are all correct as-is, unless I missed one. These all seem to operate on widgets and therefore it's more accurate to say Finder and not FinderBase.
I think we can probably close this PR and close #168230, unless there is a specific mismatch that you see in the use of Finder vs. FinderBase.
| const Matcher findsNothing = _FindsCountMatcher(null, 0); | ||
|
|
||
| /// Asserts that the [Finder] locates at least one widget in the widget tree. | ||
| /// Asserts that the [FinderBase] locates at least one widget in the widget tree. |
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.
I think this was correct as-is. findsWidgets deals with widgets and so does Finder.
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.
findsWidgets is an instance of the _FindsCountMatcher class.
This is the signature of the matches() method of _FindsCountMatcher:
bool matches(covariant FinderBase<dynamic> finder, Map<dynamic, dynamic> matchState)As you can see, it takes a FinderBase, not a Finder, as its first argument. So I think that FinderBase is the correct type that should be used in this doc comment.
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.
Ah ok you're right, they do all operate on FinderBase even when they have "widgets" in the name. I even see this below for findsOneWidget:
This is equivalent to the preferred [findsOne] method.
Indeed findsOne and findsOneWidget are identical.
| Matcher findsAtLeast(int n) => _FindsCountMatcher(n, null); | ||
|
|
||
| /// Asserts that the [Finder] locates a single widget that has at | ||
| /// Asserts that the [FinderBase] locates a single widget that has at |
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 one looks like it actually should be Finder right?
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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. This should be Finder.
| const Matcher findsNothing = _FindsCountMatcher(null, 0); | ||
|
|
||
| /// Asserts that the [Finder] locates at least one widget in the widget tree. | ||
| /// Asserts that the [FinderBase] locates at least one widget in the widget tree. |
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.
Ah ok you're right, they do all operate on FinderBase even when they have "widgets" in the name. I even see this below for findsOneWidget:
This is equivalent to the preferred [findsOne] method.
Indeed findsOne and findsOneWidget are identical.
| const Matcher isOffstage = _IsOffstage(); | ||
|
|
||
| /// Asserts that the [Finder] locates a single widget that has no | ||
| /// Asserts that the [FinderBase] locates a single widget that has no |
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.
Also this one should stay Finder.
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. This should stay Finder.
| const Matcher isInCard = _IsInCard(); | ||
|
|
||
| /// Asserts that the [Finder] locates a single widget that has no | ||
| /// Asserts that the [FinderBase] locates a single widget that has no |
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.
Should stay Finder.
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. This should stay Finder.
| /// Asserts that a [Finder], [Future<ui.Image>], or [ui.Image] matches the | ||
| /// Asserts that a [FinderBase], [Future<ui.Image>], or [ui.Image] matches the | ||
| /// golden image file identified by [key], with an optional [version] number. | ||
| /// | ||
| /// For the case of a [Finder], the [Finder] must match exactly one widget and | ||
| /// For the case of a [FinderBase], the [FinderBase] must match exactly one widget and |
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 it's a Finder:
| } else if (item is Finder) { |
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. This should stay Finder.
| const Matcher clipsWithBoundingRect = _ClipsWithBoundingRect(); | ||
|
|
||
| /// Asserts that a [Finder] locates a single object whose root RenderObject is | ||
| /// Asserts that a [FinderBase] locates a single object whose root RenderObject is |
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.
Finder:
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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. This should stay Finder.
| const Matcher hasNoImmediateClip = _MatchAnythingExceptClip(); | ||
|
|
||
| /// Asserts that a [Finder] locates a single object whose root RenderObject | ||
| /// Asserts that a [FinderBase] locates a single object whose root RenderObject |
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.
Finder:
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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. This should stay Finder.
| } | ||
|
|
||
| /// Asserts that a [Finder] locates a single object whose root RenderObject | ||
| /// Asserts that a [FinderBase] locates a single object whose root RenderObject |
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.
Finder:
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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. This should stay Finder.
| } | ||
|
|
||
| /// Asserts that a [Finder] locates a single object whose root RenderObject | ||
| /// Asserts that a [FinderBase] locates a single object whose root RenderObject |
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.
Finder:
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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, Finder.
| } | ||
|
|
||
| /// Asserts that a [Finder] locates a single object whose root RenderObject | ||
| /// Asserts that a [FinderBase] locates a single object whose root RenderObject |
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.
Finder:
| bool matches(covariant Finder finder, Map<dynamic, dynamic> matchState) { |
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, Finder.
|
I have updated the pull request based on the last investigation. |
|
@justinmc Is there any additional needed requirements to merge this PR? |
justinmc
left a comment
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 👍 Thank you for dealing with all of my nitpicks on this PR and following through! I appreciate the effort to improve our docs 🙏 .
|
This is my first approved PR in the Flutter repository. I'm happy. Thank you. |
bleroux
left a comment
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!
flutter/flutter@adffe24...ac12f66 2025-07-09 engine-flutter-autoroll@skia.org Roll Packages from cba2e90 to 4a231ae (5 revisions) (flutter/flutter#171879) 2025-07-09 ahmedsameha1@gmail.com Replace [FinderBase] with [Finder] in the documentation of Matchers (flutter/flutter#168279) 2025-07-09 mdebbar@google.com Revert "Mark web_long_running_tests_2_5 as bringup" (flutter/flutter#171872) 2025-07-09 bruno.leroux@gmail.com Apply normalization to TimePickerThemeData.inputDecorationTheme (flutter/flutter#171584) 2025-07-09 bruno.leroux@gmail.com Fix InputDecorationThemeData.activeIndicatorBorder is not applied (flutter/flutter#171764) 2025-07-09 robert.ancell@canonical.com Fix multi-view GL rendering not working since software rendering was added (flutter/flutter#171409) 2025-07-09 fluttergithubbot@gmail.com Marks Linux_android_emu android_engine_vulkan_tests to be unflaky (flutter/flutter#171141) 2025-07-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from AinHuT0vgOelA1g7_... to 0-xqmXWc4cXzw3tfe... (flutter/flutter#171823) 2025-07-08 fluttergithubbot@gmail.com Marks Linux_android_emu android_display_cutout to be unflaky (flutter/flutter#171140) 2025-07-08 1063596+reidbaker@users.noreply.github.com [Documentation] When updating kgp minimum document additional changes that are required (flutter/flutter#171819) 2025-07-08 fluttergithubbot@gmail.com Marks Linux_android_emu android_engine_opengles_tests to be unflaky (flutter/flutter#171142) 2025-07-08 biggs0125@gmail.com Add support for running dart2wasm in dry run mode on js compilations (flutter/flutter#171682) 2025-07-08 matanlurey@users.noreply.github.com Remove now duplicate un-forward ports for Android (flutter/flutter#171473) 2025-07-08 kjlubick@users.noreply.github.com [skia] Update usage of removed gn flag (flutter/flutter#171800) 2025-07-08 mdebbar@google.com [web] Disable auto-formatting for the stack_trace.dart test file (flutter/flutter#171801) 2025-07-08 1063596+reidbaker@users.noreply.github.com Bump warn and error versions of agp, kotlin and gradle versions in preparation for gradle 9 (flutter/flutter#171776) 2025-07-08 63253361+Phantom-101@users.noreply.github.com Removed string keys (flutter/flutter#171293) 2025-07-08 32538273+ValentinVignal@users.noreply.github.com Add `radioSide` to `RadioListTile` (flutter/flutter#171318) 2025-07-08 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from ZpnML-jis0gVIvtx5... to MnFlN7VWM_7h7EmBV... (flutter/flutter#171787) 2025-07-08 matanlurey@users.noreply.github.com Add/use `addMachineOutputFlag`/`outputsMachineFormat` instead of strings (flutter/flutter#171459) 2025-07-08 36861262+QuncCccccc@users.noreply.github.com Update translation from console (flutter/flutter#171556) 2025-07-08 bkonyi@google.com [ Tool ] Support upgrading to a new Flutter version pointing to the same revision as a previous version (flutter/flutter#171783) 2025-07-08 matej.knopp@gmail.com Multi-window support (engine) (flutter/flutter#168728) 2025-07-08 34269530+PrimaelQuemerais@users.noreply.github.com Fixes an issue where TapRegion would consume taps regardless of navigation state (flutter/flutter#169067) 2025-07-08 rmolivares@renzo-olivares.dev SliverSemantics (flutter/flutter#167300) 2025-07-08 engine-flutter-autoroll@skia.org Roll Skia from e159882c6ce0 to 0fef076beec3 (3 revisions) (flutter/flutter#171779) 2025-07-08 mdebbar@google.com Run hot_restart_web_amd_test.dart on Mac/Windows (flutter/flutter#171281) 2025-07-08 engine-flutter-autoroll@skia.org Roll Packages from 2c52f24 to cba2e90 (2 revisions) (flutter/flutter#171775) 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 louisehsu@google.com,stuartmorgan@google.com 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
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
flutter/flutter@adffe24...ac12f66 2025-07-09 engine-flutter-autoroll@skia.org Roll Packages from cba2e90 to 4a231ae (5 revisions) (flutter/flutter#171879) 2025-07-09 ahmedsameha1@gmail.com Replace [FinderBase] with [Finder] in the documentation of Matchers (flutter/flutter#168279) 2025-07-09 mdebbar@google.com Revert "Mark web_long_running_tests_2_5 as bringup" (flutter/flutter#171872) 2025-07-09 bruno.leroux@gmail.com Apply normalization to TimePickerThemeData.inputDecorationTheme (flutter/flutter#171584) 2025-07-09 bruno.leroux@gmail.com Fix InputDecorationThemeData.activeIndicatorBorder is not applied (flutter/flutter#171764) 2025-07-09 robert.ancell@canonical.com Fix multi-view GL rendering not working since software rendering was added (flutter/flutter#171409) 2025-07-09 fluttergithubbot@gmail.com Marks Linux_android_emu android_engine_vulkan_tests to be unflaky (flutter/flutter#171141) 2025-07-08 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from AinHuT0vgOelA1g7_... to 0-xqmXWc4cXzw3tfe... (flutter/flutter#171823) 2025-07-08 fluttergithubbot@gmail.com Marks Linux_android_emu android_display_cutout to be unflaky (flutter/flutter#171140) 2025-07-08 1063596+reidbaker@users.noreply.github.com [Documentation] When updating kgp minimum document additional changes that are required (flutter/flutter#171819) 2025-07-08 fluttergithubbot@gmail.com Marks Linux_android_emu android_engine_opengles_tests to be unflaky (flutter/flutter#171142) 2025-07-08 biggs0125@gmail.com Add support for running dart2wasm in dry run mode on js compilations (flutter/flutter#171682) 2025-07-08 matanlurey@users.noreply.github.com Remove now duplicate un-forward ports for Android (flutter/flutter#171473) 2025-07-08 kjlubick@users.noreply.github.com [skia] Update usage of removed gn flag (flutter/flutter#171800) 2025-07-08 mdebbar@google.com [web] Disable auto-formatting for the stack_trace.dart test file (flutter/flutter#171801) 2025-07-08 1063596+reidbaker@users.noreply.github.com Bump warn and error versions of agp, kotlin and gradle versions in preparation for gradle 9 (flutter/flutter#171776) 2025-07-08 63253361+Phantom-101@users.noreply.github.com Removed string keys (flutter/flutter#171293) 2025-07-08 32538273+ValentinVignal@users.noreply.github.com Add `radioSide` to `RadioListTile` (flutter/flutter#171318) 2025-07-08 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from ZpnML-jis0gVIvtx5... to MnFlN7VWM_7h7EmBV... (flutter/flutter#171787) 2025-07-08 matanlurey@users.noreply.github.com Add/use `addMachineOutputFlag`/`outputsMachineFormat` instead of strings (flutter/flutter#171459) 2025-07-08 36861262+QuncCccccc@users.noreply.github.com Update translation from console (flutter/flutter#171556) 2025-07-08 bkonyi@google.com [ Tool ] Support upgrading to a new Flutter version pointing to the same revision as a previous version (flutter/flutter#171783) 2025-07-08 matej.knopp@gmail.com Multi-window support (engine) (flutter/flutter#168728) 2025-07-08 34269530+PrimaelQuemerais@users.noreply.github.com Fixes an issue where TapRegion would consume taps regardless of navigation state (flutter/flutter#169067) 2025-07-08 rmolivares@renzo-olivares.dev SliverSemantics (flutter/flutter#167300) 2025-07-08 engine-flutter-autoroll@skia.org Roll Skia from e159882c6ce0 to 0fef076beec3 (3 revisions) (flutter/flutter#171779) 2025-07-08 mdebbar@google.com Run hot_restart_web_amd_test.dart on Mac/Windows (flutter/flutter#171281) 2025-07-08 engine-flutter-autoroll@skia.org Roll Packages from 2c52f24 to cba2e90 (2 revisions) (flutter/flutter#171775) 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 louisehsu@google.com,stuartmorgan@google.com 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
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
…lutter#168279) Replace [FinderBase] with [Finder] in the doc comments of Matchers. Closes flutter#168230
Replace [FinderBase] with [Finder] in the doc comments of Matchers.
Closes #168230