[go_router_builder] Adopt code excerpts in README #5698
Conversation
Remove 'Import' section
packages/go_router_builder/README.md
Outdated
| @@ -1,34 +1,5 @@ | |||
| ## Usage | |||
|
|
|||
| ### Dependencies | |||
There was a problem hiding this comment.
Stuart said to remove the "Import" sections from the READMEs.. I assumed that would also apply to Dependencies.
There was a problem hiding this comment.
installing https://pub.dev/packages/go_router_builder/install page seems to be generic, and does not really work for build_runner.
There was a problem hiding this comment.
I added a separate .yaml file to store the code snippet for the "Dependencies" section since the existing one uses a relative path for go_router_builder. I'm not sure if this was the best way to do it though.
There was a problem hiding this comment.
installing https://pub.dev/packages/go_router_builder/install page seems to be generic, and does not really work for build_runner.
That's dart-lang/pub-dev#3403, FYI.
There was a problem hiding this comment.
I added a separate
.yamlfile to store the code snippet for the "Dependencies" section since the existing one uses a relative path forgo_router_builder. I'm not sure if this was the best way to do it though.
This doesn't actually accomplish anything, unfortunatetly, since there's no mechanism by which a random .yaml file would be kept updated. Since we don't have an authoritative source to pull the excerpt from, you can just not except for that block. The tool doesn't enforce excerpts for yaml.
Add comment explaining purpose of readme_excerpts.dart and readme_excerpts.yaml
There was a problem hiding this comment.
Looks good to me other than some small comments, thanks! I'll leave final approval to @chunhtai since there are some changes to the example code, and I'm not familiar enough with this codebase to know how important they are to the understandability.
packages/go_router_builder/README.md
Outdated
| ```dart | ||
| import 'package:go_router/go_router.dart'; | ||
|
|
||
| part 'this_file.g.dart'; | ||
| part 'readme_excerpts.g.dart'; |
There was a problem hiding this comment.
Nit: this naming is just a convention for our convenience since it usually doesn't matter, but having it in the readme with this name seems potentially confusing to package clients. How about we call the file something like example_builder_output.g.dart?
| path: 'family/:familyId', | ||
| ) | ||
| path: 'family/:fid', | ||
| // ··· |
There was a problem hiding this comment.
Let's include the closing ) here, even though it'll mean an extra plaster. Same for similar cases later in the file; it's just less confusing if we don't have unbalanced ([{s.
packages/go_router_builder/README.md
Outdated
| @TypedGoRoute<LoginRoute>(path: '/login') | ||
| class LoginRoute extends GoRouteData {...} | ||
| @TypedGoRoute<LoginRoute>( | ||
| path: '/login', |
There was a problem hiding this comment.
Nit: Seems like we could remove the trailing comma in the source here?
| ```dart | ||
| void _tap() => PersonRoute(fid: 'f2', pid: 'p1').go(context); | ||
| onTap: () => PersonRoute(family.id, p.id).go(context), |
There was a problem hiding this comment.
I'm not familiar with the go_router code; are there both named and positional versions of the constructors? I see one below that still have the pid:.
If so, it seems like we should be using the named versions, as they are safer?
There was a problem hiding this comment.
we should probably shows the same version in both code excerpt.
@mike-v2 can you update the code sample?
There was a problem hiding this comment.
This file can be removed, per my other comment.
Rename 'readme_excerpts' generated file Fix formatting errors
| ```dart | ||
| void _tap() => PersonRoute(fid: 'f2', pid: 'p1').go(context); | ||
| onTap: () => PersonRoute(family.id, p.id).go(context), |
There was a problem hiding this comment.
we should probably shows the same version in both code excerpt.
@mike-v2 can you update the code sample?
| // #docregion Import | ||
| import 'package:go_router/go_router.dart'; | ||
|
|
||
| part 'example_builder_output.g.dart'; |
There was a problem hiding this comment.
why we use different name for the generated file?
|
Hi @mike-v2 are you still working on this pr? |
|
Closing this one for now. Feel free to reopen when you have time |
Remove 'Import' section
Updates the README to use a compiled excerpt source for its example of using go_router_builder plugin.
Part of flutter/flutter#102679
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto 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.