[go_router] Add support for Iterable, List and Set to TypedGoRoute #2698

Skogsfrae · 2022-10-10T15:05:24Z

Add support for Iterable, List and Set to TypedGoRoute

Changed signature of GoRouteData.$location for PR #2679
Resolves Issue #108437

Pre-launch Checklist

flutter-dashboard · 2022-10-10T15:05:26Z

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.

stuartmorgan-g · 2022-10-27T16:54:23Z

@chunhtai From triage: ping on this review.

chunhtai · 2022-10-27T21:41:18Z

Updates: this one is waiting on #2679

chunhtai

LGTM

chunhtai · 2022-12-01T22:51:22Z

Hi @Skogsfrae looks like there are ci failures and merge conflict. can you rebase off the latest main and fix the ci?

Skogsfrae · 2022-12-07T15:30:35Z

Hi @chunhtai I finally managed to make the ci work. I temporarily modified the analysis_options.yaml to skip go_router_builder example app .g.dart files. I'll restore it in the go_router_builder pr after this gets merged

stuartmorgan-g · 2022-12-07T15:36:50Z

analysis_options.yaml

We should not be turning off analysis for an entire directory. What problem is this intended to solve?

@stuartmorgan This pr changes the signature of a go_router method used in .g.dart generated files and causing problems in cyrrus ci analysis step when running the analyser for go_router_builder/example app.

Talking about that on discord, @chunhtai told me to temporarily disable the analysis for that example app so this pr can be merged and then reenable it in #2679
I first tried to disable the analysis only on those specific files, but then I faced another problem for updating a file of a library without increasing its version, so I had to revert it.

This are the cyrrus ci logs:
https://cirrus-ci.com/task/4546239256592384
https://cirrus-ci.com/task/6292642485501952

Running command: "dart analyze --fatal-infos" in /tmp/cirrus-ci-build/packages/go_router_builder Analyzing go_router_builder... info - example/lib/all_types.g.dart:205:61 - Unnecessary null checks. - unnecessary_null_checks info - example/lib/all_types.g.dart:226:60 - Unnecessary null checks. - unnecessary_null_checks info - example/lib/all_types.g.dart:244:63 - Unnecessary null checks. - unnecessary_null_checks info - example/lib/main.g.dart:122:54 - Unnecessary null checks. - unnecessary_null_checks 4 issues found.

if [[ $CIRRUS_PR == "" ]]; then ./script/tool_runner.sh version-check --ignore-platform-interface-breaks else ./script/tool_runner.sh version-check --check-for-missing-changes --pr-labels="$CIRRUS_PR_LABELS" fi Running for all packages that have diffs relative to "fdf708371f2e33f44189ebc43d896679b7ebf463" Changed packages: go_router,go_router_builder �[36m[0:00] Running for go_router...�[0m 5.2.0 -> 5.2.1 �[36m[0:00] Running for go_router_builder...�[0m No version change. �[31mNo CHANGELOG change found. If this PR needs an exemption from the standard policy of listing all changes in the CHANGELOG, comment in the PR to explain why the PR is exempt, and add (or ask your reviewer to add) the "override: no changelog needed" label. Otherwise, please add a NEXT entry in the CHANGELOG as described in the contributing guide.�[0m �[31mThe following packages had errors:�[0m �[31m go_router_builder: Missing CHANGELOG change�[0m �[31mSee above for full details.�[0m

then I faced another problem for updating a file of a library without increasing its version, so I had to revert it.

The second failure you linked to is only for not having a CHANGELOG entry, not lack of version update, and as the error message says that's trivially overridable when it makes sense to do so (as in this case). Local ignores and an override for the changelog would be much better than changing the root analysis configuration to opt everything out.

Sorry for that, I genuinely thought it was a versioning problem.
I fixed the pr, can you add the override: no changelog needed label? I can't do it

Skogsfrae · 2022-12-12T13:09:48Z

If someone can please add the override: no changelog needed I'll update the pr to solve the conflicts and the CI could run without the previous changelog problem

stuartmorgan-g · 2022-12-12T13:56:04Z

Overriding for go_router_builder; ignores are dev-only changes.

Skogsfrae · 2022-12-19T08:50:40Z

@chunhtai this is ready to be merged

stuartmorgan-g · 2023-01-17T21:04:51Z

@johnpryan Who should do the second review on this?

stuartmorgan-g · 2023-01-31T20:39:48Z

@chunhtai It looks like you reviewed this before, could you re-review as the second reviewer?

chunhtai

LGTM

chunhtai · 2023-01-31T22:04:09Z

@Skogsfrae can you rebase off latest main?

Skogsfrae · 2023-02-08T08:51:11Z

@chunhtai I rebased both this pr and the one on go_router_builder

Skogsfrae · 2023-02-14T09:33:22Z

@chunhtai I just rebased it again with the last release. Is there something else that needs to be done to merge this PR?

…e generator

…lder code generator" This reverts commit 6ae5026c0df976c6003b1e4aee10ed95c6601783.

This reverts commit 48bdeb5abba23847e90ec0ba3c788b279e87eddb.

…alysis checks

… global analysis

Skogsfrae · 2023-02-16T17:40:01Z

@chunhtai @stuartmorgan @johnpryan can this PR be merged so I can update the linked one and hopefully have that merged too?
I'm keeping this updated with upstream and I'd really like to be able to use this feature with go_router_builder

Thank you guys 🙏🏼

stuartmorgan-g · 2023-02-16T17:51:24Z

Based on the last comments from reviewers it looks like this is ready to land and just didn't get the label.

flutter-dashboard bot added the needs tests label Oct 10, 2022

github-actions bot added the p: go_router label Oct 10, 2022

chunhtai self-requested a review October 13, 2022 21:45

chunhtai approved these changes Nov 21, 2022

View reviewed changes

chunhtai mentioned this pull request Nov 21, 2022

[go_router_builder] Add support for Iterable, List and Set to TypedGoRoute #2679

Merged

11 tasks

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from 075757d to b68fa74 Compare November 22, 2022 15:19

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

chunhtai requested a review from johnpryan December 1, 2022 22:50

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from aa84861 to 173e37a Compare December 7, 2022 14:23

github-actions bot removed the p: go_router_builder label Dec 7, 2022

stuartmorgan-g reviewed Dec 7, 2022

View reviewed changes

github-actions bot added the p: go_router_builder label Dec 7, 2022

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

stuartmorgan-g added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Dec 12, 2022

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from 3217a65 to 691a372 Compare December 12, 2022 16:38

johnpryan approved these changes Dec 19, 2022

View reviewed changes

chunhtai approved these changes Jan 31, 2023

View reviewed changes

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from 691a372 to 262df99 Compare February 7, 2023 14:01

github-actions bot removed the p: go_router_builder label Feb 7, 2023

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from 28e55cd to eacf2f2 Compare February 14, 2023 08:26

Skogsfrae added 8 commits February 16, 2023 18:02

fixes #108437 support for iterable, list and set in for TypedGoRoute

1717a48

temporary fix for analyzer ci, will be fixed in go_router_builder cod…

b3bddbc

…e generator

Revert "temporary fix for analyzer ci, will be fixed in go_router_bui…

579af61

…lder code generator" This reverts commit 6ae5026c0df976c6003b1e4aee10ed95c6601783.

added temporary ignore directive

5ed42ea

Revert "added temporary ignore directive"

92a3d6e

This reverts commit 48bdeb5abba23847e90ec0ba3c788b279e87eddb.

fix merge conflict and temoporary remove go_router_builder/example an…

f052106

…alysis checks

fix temoporary go_router_builder/example analysis checks and restored…

d2a6ad3

… global analysis

fix ci

fc79f9c

Skogsfrae force-pushed the feature/#108437_go_router-support-for-iterables branch from eacf2f2 to fc79f9c Compare February 16, 2023 17:03

version

a50d46a

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 16, 2023

auto-submit bot merged commit fa7cdfa into flutter:main Feb 16, 2023

[go_router] Add support for Iterable, List and Set to TypedGoRoute #2698

[go_router] Add support for Iterable, List and Set to TypedGoRoute #2698

Uh oh!

Conversation

Skogsfrae commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Oct 10, 2022

Uh oh!

stuartmorgan-g commented Oct 27, 2022

Uh oh!

chunhtai commented Oct 27, 2022

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Dec 1, 2022

Uh oh!

Skogsfrae commented Dec 7, 2022

Uh oh!

stuartmorgan-g Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Skogsfrae Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Skogsfrae Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

Skogsfrae commented Dec 12, 2022

Uh oh!

stuartmorgan-g commented Dec 12, 2022

Uh oh!

Skogsfrae commented Dec 19, 2022

Uh oh!

stuartmorgan-g commented Jan 17, 2023

Uh oh!

stuartmorgan-g commented Jan 31, 2023

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jan 31, 2023

Uh oh!

Skogsfrae commented Feb 8, 2023

Uh oh!

Skogsfrae commented Feb 14, 2023

Uh oh!

Skogsfrae commented Feb 16, 2023

Uh oh!

stuartmorgan-g commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Skogsfrae commented Oct 10, 2022 •

edited

Loading