[go_router_builder] Generate replace method in RouteExtension #2838

omensight · 2022-11-21T21:47:30Z

The problem is described here flutter/flutter#115789
The method replace is added to the route_config.dart file

List which issues are fixed by this PR. You must list at least one issue.
The replace method is needed in the generated class

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-11-21T21:47:32Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

omensight · 2022-11-21T21:57:19Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

The test

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

The ShouldGenerate annotation method have been edited in the line 93 of the file "_go_router_builder_test_input.dart" before to edit the lib code

chunhtai

you should probably need run code gen in the example/ folder. I am surprise this does not trigger ci failure. The test/ensure_build_test.dart should have failed. @kevmoo Since you wrote the package that enabled this test, do you know why this is the case

kevmoo · 2022-11-28T21:21:40Z

https://cirrus-ci.com/task/4668337259544576?logs=unit_test#L239 This test is not properly configured on CI to run!

…

On Mon, Nov 28, 2022 at 1:18 PM chunhtai ***@***.***> wrote: ***@***.**** commented on this pull request. you should probably need run code gen in the example/ folder. I am surprise this does not trigger ci failure. The test/ensure_build_test.dart should have failed. @kevmoo <https://github.com/kevmoo> Since you wrote the package that enabled this test, do you know why this is the case — Reply to this email directly, view it on GitHub <#2838 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCV7ZAGBLR674U5JHALWKUOQ3ANCNFSM6AAAAAASHC7D7M> . You are receiving this because you were mentioned.Message ID: ***@***.***>

chunhtai · 2022-11-28T21:25:32Z

lol it looks like the presubmit-only tag is not working properly

kevmoo · 2022-11-28T21:31:18Z

lol it looks like the presubmit-only tag is not working properly

You need to pass a flag to the test command to include the ensure bits.

See https://github.com/google/json_serializable.dart/blob/master/.github/workflows/dart.yml#L335

chunhtai · 2022-11-28T21:48:48Z

This will requires flutter_plugin_tools to support adding -t to the test command. another question may be whether this this test really needs this tag, Is there a reason why this need to be presubmit only?

kevmoo · 2022-11-29T00:53:03Z

@chunhtai – it runs a build and checks for a git delta. Can be annoying during normal development.

We could certainly drop it in the short run!

omensight · 2022-12-02T14:18:59Z

Contributor

What should I do for it to be accepted?

chunhtai · 2022-12-15T22:45:59Z

We should wait for #2846 (comment) because we are going to rename this to pushReplacement

chunhtai · 2023-01-04T18:05:49Z

can you rebase off latest main and change the name to pushReplacement

stuartmorgan · 2023-01-24T21:24:45Z

What's the status of this PR? Is it ready for re-review?

chunhtai

LGTM

Hixie · 2023-01-31T22:34:58Z

Should we test that the generated files are the files you get when you run the generator? Does that make sense?

chunhtai · 2023-01-31T22:54:53Z

Should we test that the generated files are the files you get when you run the generator? Does that make sense?

Yes there is a test that makes sure of that https://github.com/flutter/packages/blob/main/packages/go_router_builder/example/test/ensure_build_test.dart

Hixie · 2023-02-01T19:09:53Z

test-exempt: is tested

hangyujin

LGTM

chunhtai · 2023-02-08T19:52:48Z

hi @omensight can you fix the merge conflict?

omensight · 2023-02-15T01:45:23Z

hi @omensight can you fix the merge conflict?

I did it, thanks for waiting.

chunhtai · 2023-02-16T22:46:53Z

@omensight it looks like there are some test failures

omensight · 2023-02-17T03:52:27Z

@omensight it looks like there are some test failures

I'm sorry but I'm not understanding what the problem is.

chunhtai · 2023-02-17T20:57:35Z

This is the test failure

test/builder_test.dart: DefaultValueRoute                                                                                                                                                    
00:34 
+2
 -1
: test/builder_test.dart: DefaultValueRoute 
[E]
  Expected: 'GoRoute get $defaultValueRoute => GoRouteData.$route(\n'
              '      path: \'/default-value-route\',\n'
              '      factory: $DefaultValueRouteExtension._fromState,\n'
              '    );\n'
              '\n'
              'extension $DefaultValueRouteExtension on DefaultValueRoute {\n'
              '  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(\n'
              '        param: _$convertMapValue(\'param\', state.queryParams, int.parse) ?? 0,\n'
              '      );\n'
              '\n'
              '  String get location => GoRouteData.$location(\n'
              '        \'/default-value-route\',\n'
              '        queryParams: {\n'
              '          if (param != 0) \'param\': param.toString(),\n'
              '        },\n'
              '      );\n'
              '\n'
              '  void go(BuildContext context) => context.go(location, extra: this);\n'
              '\n'
              '  void push(BuildContext context) => context.push(location, extra: this);\n'
              '}\n'
              '\n'
              'T? _$convertMapValue<T>(\n'
              '  String key,\n'
              '  Map<String, String> map,\n'
              '  T Function(String) converter,\n'
              ') {\n'
              '  final value = map[key];\n'
              '  return value == null ? null : converter(value);\n'
              '}\n'
              ''
    Actual: 'GoRoute get $defaultValueRoute => GoRouteData.$route(\n'
              '      path: \'/default-value-route\',\n'
              '      factory: $DefaultValueRouteExtension._fromState,\n'
              '    );\n'
              '\n'
              'extension $DefaultValueRouteExtension on DefaultValueRoute {\n'
              '  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(\n'
              '        param: _$convertMapValue(\'param\', state.queryParams, int.parse) ?? 0,\n'
              '      );\n'
              '\n'
              '  String get location => GoRouteData.$location(\n'
              '        \'/default-value-route\',\n'
              '        queryParams: {\n'
              '          if (param != 0) \'param\': param.toString(),\n'
              '        },\n'
              '      );\n'
              '\n'
              '  void go(BuildContext context) => context.go(location, extra: this);\n'
              '\n'
              '  void push(BuildContext context) => context.push(location, extra: this);\n'
              '\n'
              '  void pushReplacement(BuildContext context) =>\n'
              '      context.pushReplacement(location, extra: this);\n'
              '}\n'
              '\n'
              'T? _$convertMapValue<T>(\n'
              '  String key,\n'
              '  Map<String, String> map,\n'
              '  T Function(String) converter,\n'
              ') {\n'
              '  final value = map[key];\n'
              '  return value == null ? null : converter(value);\n'
              '}\n'
              ''
     Which: is different.
            Expected: ... : this);\n}\n\nT? _$ ...
              Actual: ... : this);\n\n  void p ...
                                    ^
             Differ at offset 725
  package:test_api                                               expect
  package:source_gen_test/src/test_annotated_classes.dart 244:7  AnnotatedTest._shouldGenerateTest
ACTUAL CONTENT:
r'''
GoRoute get $defaultValueRoute => GoRouteData.$route(
      path: '/default-value-route',
      factory: $DefaultValueRouteExtension._fromState,
    );
extension $DefaultValueRouteExtension on DefaultValueRoute {
  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(
        param: _$convertMapValue('param', state.queryParams, int.parse) ?? 0,
      );
  String get location => GoRouteData.$location(
        '/default-value-route',
        queryParams: {
          if (param != 0) 'param': param.toString(),
        },
      );
  void go(BuildContext context) => context.go(location, extra: this);
  void push(BuildContext context) => context.push(location, extra: this);
  void pushReplacement(BuildContext context) =>
      context.pushReplacement(location, extra: this);
}
T? _$convertMapValue<T>(
  String key,
  Map<String, String> map,
  T Function(String) converter,
) {
  final value = map[key];
  return value == null ? null : converter(value);
}
'''

omensight · 2023-02-18T22:32:47Z

This is the test failure

test/builder_test.dart: DefaultValueRoute                                                                                                                                                    
00:34 
+2
 -1
: test/builder_test.dart: DefaultValueRoute 
[E]
  Expected: 'GoRoute get $defaultValueRoute => GoRouteData.$route(\n'
              '      path: \'/default-value-route\',\n'
              '      factory: $DefaultValueRouteExtension._fromState,\n'
              '    );\n'
              '\n'
              'extension $DefaultValueRouteExtension on DefaultValueRoute {\n'
              '  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(\n'
              '        param: _$convertMapValue(\'param\', state.queryParams, int.parse) ?? 0,\n'
              '      );\n'
              '\n'
              '  String get location => GoRouteData.$location(\n'
              '        \'/default-value-route\',\n'
              '        queryParams: {\n'
              '          if (param != 0) \'param\': param.toString(),\n'
              '        },\n'
              '      );\n'
              '\n'
              '  void go(BuildContext context) => context.go(location, extra: this);\n'
              '\n'
              '  void push(BuildContext context) => context.push(location, extra: this);\n'
              '}\n'
              '\n'
              'T? _$convertMapValue<T>(\n'
              '  String key,\n'
              '  Map<String, String> map,\n'
              '  T Function(String) converter,\n'
              ') {\n'
              '  final value = map[key];\n'
              '  return value == null ? null : converter(value);\n'
              '}\n'
              ''
    Actual: 'GoRoute get $defaultValueRoute => GoRouteData.$route(\n'
              '      path: \'/default-value-route\',\n'
              '      factory: $DefaultValueRouteExtension._fromState,\n'
              '    );\n'
              '\n'
              'extension $DefaultValueRouteExtension on DefaultValueRoute {\n'
              '  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(\n'
              '        param: _$convertMapValue(\'param\', state.queryParams, int.parse) ?? 0,\n'
              '      );\n'
              '\n'
              '  String get location => GoRouteData.$location(\n'
              '        \'/default-value-route\',\n'
              '        queryParams: {\n'
              '          if (param != 0) \'param\': param.toString(),\n'
              '        },\n'
              '      );\n'
              '\n'
              '  void go(BuildContext context) => context.go(location, extra: this);\n'
              '\n'
              '  void push(BuildContext context) => context.push(location, extra: this);\n'
              '\n'
              '  void pushReplacement(BuildContext context) =>\n'
              '      context.pushReplacement(location, extra: this);\n'
              '}\n'
              '\n'
              'T? _$convertMapValue<T>(\n'
              '  String key,\n'
              '  Map<String, String> map,\n'
              '  T Function(String) converter,\n'
              ') {\n'
              '  final value = map[key];\n'
              '  return value == null ? null : converter(value);\n'
              '}\n'
              ''
     Which: is different.
            Expected: ... : this);\n}\n\nT? _$ ...
              Actual: ... : this);\n\n  void p ...
                                    ^
             Differ at offset 725
  package:test_api                                               expect
  package:source_gen_test/src/test_annotated_classes.dart 244:7  AnnotatedTest._shouldGenerateTest
ACTUAL CONTENT:
r'''
GoRoute get $defaultValueRoute => GoRouteData.$route(
      path: '/default-value-route',
      factory: $DefaultValueRouteExtension._fromState,
    );
extension $DefaultValueRouteExtension on DefaultValueRoute {
  static DefaultValueRoute _fromState(GoRouterState state) => DefaultValueRoute(
        param: _$convertMapValue('param', state.queryParams, int.parse) ?? 0,
      );
  String get location => GoRouteData.$location(
        '/default-value-route',
        queryParams: {
          if (param != 0) 'param': param.toString(),
        },
      );
  void go(BuildContext context) => context.go(location, extra: this);
  void push(BuildContext context) => context.push(location, extra: this);
  void pushReplacement(BuildContext context) =>
      context.pushReplacement(location, extra: this);
}
T? _$convertMapValue<T>(
  String key,
  Map<String, String> map,
  T Function(String) converter,
) {
  final value = map[key];
  return value == null ? null : converter(value);
}
'''

I have fixed the error. Sorry, I didn't add the respective lines for the test.

* ad48ee5 [go_router] Fix some broken links in doc (flutter/packages#3288) * 58ac45e [go_router_builder] Add support for Iterable, List and Set to TypedGoRoute (flutter/packages#2679) * 0edae25 Export super types in route_data.dart library (flutter/packages#3286) * 8ad3fde [go_router] Add `GoRouter.maybeOf` (flutter/packages#3216) * 13ee644 [flutter_migrate] Skip slow tests (flutter/packages#3270) * 3cc754a Update .gitignore with missing values from flutter/plugins (flutter/packages#3265) * c0f0a22 [ci] Re-enable pathified unit tests (flutter/packages#3268) * 5834b4c [go_router]: implemented helpers for ShellRoute (flutter/packages#2730) * af5906b [extension_gsi] Update extension to support gsi 5 and 6. (flutter/packages#3235) * 195f4e8 Merge in plugin README and CONTRIBUTING (flutter/packages#3252) * fab47af [go_router] Disable logging in tests (flutter/packages#3263) * 25f0f70 [various] Update flutter/plugins links (flutter/packages#3256) * 2e16733 Merge flutter/plugins (flutter/packages#3233) * 324a7f2 Exclude more tests on Windows * 334b58e Adjust test configs * 69e6dac [go_router_builder] Generate replace method in RouteExtension (flutter/packages#2838) * 193e454 Merge repository metadata * 18715d7 Merge remote-tracking branch 'plugins-packages/main' into merge-flutter-plugins * f2d802d Roll Flutter from ae8d051 to 7175de4 (4 revisions) (flutter/packages#3232) * 6f1b1e8 Roll Flutter from 33e4d21 to ae8d051 (6 revisions) (flutter/packages#3229) * a162a98 Roll Flutter from 0be7c3f to 33e4d21 (5 revisions) (flutter/packages#3227) * ab5a8c0 [tool] Allow importing packages with NEXT (flutter/packages#3215) * ce9c61b Roll Flutter from 170539f to 0be7c3f (38 revisions) (flutter/packages#3225) * 9747469 Fix deprecation message for GoRouterState.namedLocation (flutter/packages#3092) * 6e4431f [go_router] Bump example `compileSdkVersion` and `package_info_plus` dependency version (flutter/packages#3219) * 925bea8 [pigeon] Validate generated files in CI (flutter/packages#3224) * 3094867 Move iOS Swift unit tests back to Cirrus (flutter/packages#3221) * 763d025 [pigeon] Eliminate some of the test pigeons (flutter/packages#3213)

omensight requested a review from chunhtai as a code owner November 21, 2022 21:47

flutter-dashboard bot added the needs tests label Nov 21, 2022

github-actions bot added the p: go_router_builder label Nov 21, 2022

omensight changed the title ~~[go_router_build] Generate replace method in RouteExtension~~ [go_router_builder] Generate replace method in RouteExtension Nov 21, 2022

chunhtai reviewed Nov 28, 2022

View reviewed changes

chunhtai self-requested a review December 8, 2022 22:58

omensight requested review from yjbanov, tarrinneal, gaaclarke, GaryQian, domesticmouse and Piinks as code owners January 4, 2023 20:51

github-actions bot added p: dynamic_layouts The dynamic_layouts package p: flutter_markdown p: flutter_migrate p: go_router p: pigeon p: web_benchmarks labels Jan 4, 2023

omensight closed this Jan 5, 2023

omensight force-pushed the main branch from 5ebd52b to 3ef0f63 Compare January 5, 2023 13:27

github-actions bot removed the p: flutter_markdown label Jan 5, 2023

[go_router_builder] pushReplacement method added

d40ca6b

omensight reopened this Jan 5, 2023

github-actions bot added the p: go_router_builder label Jan 5, 2023

chunhtai removed request for domesticmouse, yjbanov, GaryQian, Piinks, gaaclarke and tarrinneal January 5, 2023 17:06

chunhtai approved these changes Jan 31, 2023

View reviewed changes

chunhtai requested a review from hangyujin January 31, 2023 21:34

hangyujin approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into main

35c59c5

[go_router_builder] pushReplacement method added to builder test input

5c66229

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 21, 2023

auto-submit bot merged commit 69e6dac into flutter:main Feb 21, 2023

danagbemava-nc mentioned this pull request Feb 23, 2023

[go_router_builder] Generate replace method flutter/flutter#115789

Closed

ditman mentioned this pull request Feb 24, 2023

Roll Packages from 6c04c745b to ad48ee552 (28 revisions) flutter/flutter#121432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go_router_builder] Generate replace method in RouteExtension #2838

[go_router_builder] Generate replace method in RouteExtension #2838

omensight commented Nov 21, 2022 •

edited

Loading

flutter-dashboard bot commented Nov 21, 2022

omensight commented Nov 21, 2022

chunhtai left a comment

kevmoo commented Nov 28, 2022 via email

chunhtai commented Nov 28, 2022

kevmoo commented Nov 28, 2022

chunhtai commented Nov 28, 2022

kevmoo commented Nov 29, 2022

omensight commented Dec 2, 2022

chunhtai commented Dec 15, 2022

chunhtai commented Jan 4, 2023

stuartmorgan commented Jan 24, 2023

chunhtai left a comment

Hixie commented Jan 31, 2023

chunhtai commented Jan 31, 2023

Hixie commented Feb 1, 2023

hangyujin left a comment

chunhtai commented Feb 8, 2023

omensight commented Feb 15, 2023

chunhtai commented Feb 16, 2023 •

edited

Loading

omensight commented Feb 17, 2023

chunhtai commented Feb 17, 2023

omensight commented Feb 18, 2023 •

edited

Loading

[go_router_builder] Generate replace method in RouteExtension #2838

[go_router_builder] Generate replace method in RouteExtension #2838

Conversation

omensight commented Nov 21, 2022 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Nov 21, 2022

omensight commented Nov 21, 2022

chunhtai left a comment

Choose a reason for hiding this comment

kevmoo commented Nov 28, 2022 via email

chunhtai commented Nov 28, 2022

kevmoo commented Nov 28, 2022

chunhtai commented Nov 28, 2022

kevmoo commented Nov 29, 2022

omensight commented Dec 2, 2022

chunhtai commented Dec 15, 2022

chunhtai commented Jan 4, 2023

stuartmorgan commented Jan 24, 2023

chunhtai left a comment

Choose a reason for hiding this comment

Hixie commented Jan 31, 2023

chunhtai commented Jan 31, 2023

Hixie commented Feb 1, 2023

hangyujin left a comment

Choose a reason for hiding this comment

chunhtai commented Feb 8, 2023

omensight commented Feb 15, 2023

chunhtai commented Feb 16, 2023 • edited Loading

omensight commented Feb 17, 2023

chunhtai commented Feb 17, 2023

omensight commented Feb 18, 2023 • edited Loading

omensight commented Nov 21, 2022 •

edited

Loading

chunhtai commented Feb 16, 2023 •

edited

Loading

omensight commented Feb 18, 2023 •

edited

Loading