[google_maps_flutter] Switch app-facing package to new analysis options #4716

TahaTesser · 2022-01-31T18:32:52Z

Switches from legacy analysis options to current analysis options,
fixing all analysis issues it exposed.

Pre-launch Checklist

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

stuartmorgan-g · 2022-01-31T18:47:41Z

We do not want custom analysis rules in flutter/plugins. See flutter/flutter#76229 if you are interested in fixing analysis issues in the maps plugin, as that's the direction we want to go.

If there are things in flutter_lints that aren't in the repository settings, we should evaluate adding them there so the apply to all code.

TahaTesser · 2022-01-31T21:23:54Z

@stuartmorgan
Does it mean I update all the 3 maps packages in https://github.com/flutter/plugins/tree/main/packages/google_maps_flutter?

Since custom_analysis.yaml has entire google_maps_flutter, not just its example
I found that i need to update interface package too since just migrating google_maps_flutter causes the tests to fail as it expects certain types during the test.

stuartmorgan-g · 2022-01-31T21:32:42Z

Does it mean I update the all the 3 maps packages in https://github.com/flutter/plugins/tree/main/packages/google_maps_flutter?

Since custom_analysis.yaml has entire google_maps_flutter, not just its example

You need to do the entire package, not just the example. You don't need to do all three at once (and ideally shouldn't).

I found that i need to update interface package too since just migrating google_maps_flutter causes the tests to fail as it expects certain types during the test.

I'm not sure what this means. What runtime type changes were you making?

stuartmorgan-g · 2022-01-31T23:22:42Z

probably because i have assigned types to quite a few maps in the codebase

If this includes method channel code, you need to be very careful do do the right combinations of as with dynamic and then casting.

TahaTesser · 2022-02-14T15:11:33Z

@stuartmorgan
Following the instructions, I've made the changes. If it looks fine, I would move to other packages ASAP :)

stuartmorgan-g · 2022-02-14T15:43:07Z

It looks like this doesn't pass tests currently.

# Conflicts: # script/configs/custom_analysis.yaml

TahaTesser · 2022-02-17T19:20:50Z

Hey @stuartmorgan
Can you please take a look, tests are passing except publishable and submit queue? I tried providing a changelog but still fails.

TahaTesser · 2022-02-18T20:26:04Z

@stuartmorgan
fixed the changelog :)

stuartmorgan-g

Thanks for this cleanup! Just a few small things.

packages/google_maps_flutter/google_maps_flutter/AUTHORS

packages/google_maps_flutter/google_maps_flutter/example/lib/scrolling_map.dart

packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml

packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart

stuartmorgan-g · 2022-02-25T16:47:28Z

packages/google_maps_flutter/google_maps_flutter/test/polygon_updates_test.dart

    await tester.pumpWidget(_mapWithPolygons(<Polygon>{p1}));

-    p1.points.add(const LatLng(1.0, 1.0));
+    _points.add(const LatLng(1.0, 1.0));


What was this set of changes fixing? The code seems less clear now, so I'm wondering if we can fix it differently.

I meant to add final List<LatLng> _points = <LatLng>[const LatLng(0.0, 0.0)]; to p1, to make _points list modifiable
Otherwise, it throws Unsupported operation: Cannot add to an unmodifiable list

During local testing, I modified this line and probably forget to revert this line after making the changes above, thanks

+ final List<LatLng> _points = <LatLng>[const LatLng(0.0, 0.0)]; final Polygon p1 = Polygon( - polylineId: PolylineId("polyline_1"), + polygonId: const PolygonId('polygon_1'), - points: <LatLng>[const LatLng(0.0, 0.0)], + points: _points, ); await tester.pumpWidget(_mapWithPolygons(<Polygon>{p1})); p1.points.add(const LatLng(1.0, 1.0));

I see, the reason this code looks strange is because it is; looking at the blame this is testing that we notice if someone modifies what is supposed to be an immutable object.

packages/google_maps_flutter/google_maps_flutter/test/polyline_updates_test.dart

stuartmorgan-g

LGTM, thanks!

stuartmorgan-g · 2022-02-28T19:29:00Z

packages/google_maps_flutter/google_maps_flutter/test/polygon_updates_test.dart

    await tester.pumpWidget(_mapWithPolygons(<Polygon>{p1}));

-    p1.points.add(const LatLng(1.0, 1.0));
+    _points.add(const LatLng(1.0, 1.0));


I see, the reason this code looks strange is because it is; looking at the blame this is testing that we notice if someone modifies what is supposed to be an immutable object.

…ysis options (flutter/plugins#4716)

TahaTesser requested a review from cyanglaz as a code owner January 31, 2022 18:32

github-actions bot added the p: google_maps_flutter label Jan 31, 2022

TahaTesser mentioned this pull request Jan 31, 2022

[plugin][examples] Add flutter_lints to the example and fix lint issues flutter/flutter#97532

Closed

2 tasks

TahaTesser requested a review from stuartmorgan-g January 31, 2022 18:36

This comment was marked as resolved.

Sign in to view

stuartmorgan-g marked this pull request as draft February 4, 2022 21:35

stuartmorgan-g removed request for cyanglaz and stuartmorgan-g February 8, 2022 16:55

TahaTesser added 2 commits February 13, 2022 19:59

lint fixes from instructions

a3622ef

Updat changelog

bdda600

TahaTesser changed the title ~~[google_maps_flutter] Add flutter_lints to the example and fix lint issues~~ [google_maps_flutter] Switch app-facing package to new analysis options Feb 13, 2022

TahaTesser requested a review from stuartmorgan-g February 14, 2022 08:17

TahaTesser added 7 commits February 17, 2022 18:00

fix maps integration test

17a4eeb

Merge branch 'main' of github.com:flutter/plugins into maps_lint_fixes

9be3bff

# Conflicts: # script/configs/custom_analysis.yaml

resolve conflict

1fe70dd

Bump version

7827678

fix changelog

31a8487

update changelog

8bf2c41

fix version

d091a34

version bump

d7e1e05

TahaTesser marked this pull request as ready for review February 18, 2022 20:01

TahaTesser added 2 commits February 18, 2022 22:05

add next version note

65ccb90

Change NEXT to new version

20adc89

stuartmorgan-g suggested changes Feb 25, 2022

View reviewed changes

fixes form review

4266f93

TahaTesser requested a review from stuartmorgan-g February 25, 2022 21:26

stuartmorgan-g approved these changes Feb 28, 2022

View reviewed changes

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Mar 1, 2022

fluttergithubbot merged commit 868086b into flutter:main Mar 1, 2022

TahaTesser deleted the maps_lint_fixes branch March 2, 2022 08:13

engine-flutter-autoroll mentioned this pull request Mar 2, 2022

Roll Plugins from 4eb17696ce2d to 0283a99ce39d (8 revisions) flutter/flutter#99402

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2022

868086b62 [google_maps_flutter] Switch app-facing package to new anal…

b52b855

…ysis options (flutter/plugins#4716)

[google_maps_flutter] Switch app-facing package to new analysis options #4716

[google_maps_flutter] Switch app-facing package to new analysis options #4716

Uh oh!

Conversation

TahaTesser commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented Jan 31, 2022

Uh oh!

TahaTesser commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 31, 2022

Uh oh!

This comment was marked as resolved.

stuartmorgan-g commented Jan 31, 2022

Uh oh!

TahaTesser commented Feb 14, 2022

Uh oh!

stuartmorgan-g commented Feb 14, 2022

Uh oh!

TahaTesser commented Feb 17, 2022

Uh oh!

TahaTesser commented Feb 18, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

TahaTesser Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TahaTesser commented Jan 31, 2022 •

edited

Loading

TahaTesser commented Jan 31, 2022 •

edited

Loading

TahaTesser Feb 25, 2022 •

edited

Loading