-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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] New feature improve debug full path #6714
[go_router] New feature improve debug full path #6714
Conversation
- Fixed Expected Logs of Logging Test. - Fixed Expected Logs of Configuration Test.
[go_router] Feature Debug Full Path
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
…oaeb/packages into feature-debug-full-path
- Fixed Expected Logs of Logging Test. - Fixed Expected Logs of Configuration Test.
…oaeb/packages into feature-debug-full-path
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 is so so much better than the previous, Thanks a lot!!
_debugFullPathsFor(route.routes, fullPath, depth + 1, sb); | ||
path = concatenatePaths(parentFullpath, route.path); | ||
final String screenName = | ||
route.builder?.runtimeType.toString().split('=> ').last ?? ''; |
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.
Wouldn't this always be Widget
?
maybe just use GoRoute
and GoRoute(redirectOnly)
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.
screenName is the name of the widget. it will not always print Widget
. Otherwise it was for no use.
Just like in the logging_test.dart
test it logs / (Text)
Example from my app:
[GoRouter] Full paths for routes:
├─/ (SplashScreen)
├─/intro (IntroScreen)
├─ (ShellRoute)
│ ├─/main/year ()
│ │ └─/main/year/month (HomeScreen)
│ └─/main/profile (ProfileScreen)
│ ├─/main/profile/analytics (AnalyticsScreen)
│ └─/main/profile/settings (SettingsScreen)
│ └─/main/profile/settings/license (LicenseAgreementScreen)
└─/journal (JournalsListScreen)
├─/journal/new (JournalCreateScreen)
└─/journal/:id (JournalDetailScreen)
└─/journal/:id/edit (JournalEditScreen)
There are edge cases:
- screenName is only get printed, when user uses
GoRouter.builder
- It does not print for
GoRouter.pageBuilder
- It prints
(Never)
whenGoRouter.builder
throwsUnimplemented Error
instead of using magic strings final String newDecoration =
parentDecoration.replaceAll('├─', '│ ').replaceAll('└─', ' '); we can treat final newDecoration = parentDecoration.map((e) {
switch (e) {
case DecorType.branch:
return DecorType.parentBranch;
case DecorType.leaf:
return DecorType.none;
default:
return e;
}
}); where DecorType is enum DecorType {
parentBranch('| '),
branch('├─'),
leaf('└─'),
none(' '),
;
final String value;
const DecorType(this.value);
@override
String toString() => value;
} @chunhtai What are your thoughts, is it worth changing? |
yup that sounds better |
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
Just checking. If anything is pending on my side, just let me know. @chunhtai @hangyujin. Looking forward to the merge. 🙈 |
LGTM, this looks very helpful! |
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
This PR fixes flutter/flutter#148121 - Replaced `=>` with `| `, `��` and `�� ` to improve readability - It prints the Widget name for easy referencing - Shell routes does not have their own paths for it is presented as ` (Shell route)` in the tree - Prints the widget name of the routes it is building.
flutter/packages@ba19b24...6525441 2024-05-22 stuartmorgan@google.com [local_auth] Convert native unit tests to Swift (flutter/packages#6779) 2024-05-22 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.5.0 to 1.8.0 in /packages/interactive_media_ads/android (flutter/packages#6771) 2024-05-22 vongrejadam@gmail.com [in_app_purchase_android] Introduced new ReplacementMode for Android's billing client (flutter/packages#6515) 2024-05-21 hashirshoaeb@gmail.com [go_router] New feature improve debug full path (flutter/packages#6714) 2024-05-21 stuartmorgan@google.com [interactive_media_ads] Add SPM support (flutter/packages#6756) 2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter from 02a6c91 to d02292d (22 revisions) (flutter/packages#6778) 2024-05-21 stuartmorgan@google.com [local_auth] Remove use of OCMock (flutter/packages#6757) 2024-05-21 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.3 to 3.25.6 (flutter/packages#6777) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/file_selector/file_selector_android/android (flutter/packages#6769) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#6765) 2024-05-20 49699333+dependabot[bot]@users.noreply.github.com [url_launcher]: Bump androidx.annotation:annotation from 1.7.1 to 1.8.0 in /packages/url_launcher/url_launcher_android/android (flutter/packages#6762) 2024-05-20 engine-flutter-autoroll@skia.org Roll Flutter from adf279f to 02a6c91 (8 revisions) (flutter/packages#6776) 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,rmistry@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
This PR fixes flutter/flutter#148121
=>
with|
,├─
and└─
to improve readability(Shell route)
in the treePre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.