[various] More analysis_options alignment #6949
Conversation
stuartmorgan-g
requested review from
GaryQian,
bparrishMines,
camsim99,
cbracken,
cyanglaz,
ditman,
hellohuanlin,
jmagman and
tarrinneal
as code owners
github-actions
bot
added
p: camera
p: google_maps_flutter
p: in_app_purchase
p: shared_preferences
p: url_launcher
p: webview_flutter
|
This just needs one person to review the whole PR; there's nothing complex here that requires each plugin owner to review their slice. |
|
Seems like you broke a couple tests in the process here. Also readme excerpts need an update |
| // ignore: body_might_complete_normally_catch_error | ||
| (Object error, StackTrace stackTrace) { | ||
| if (error is! PlatformException) { | ||
| // ignore: only_throw_errors |
There was a problem hiding this comment.
Should these be addressed instead of ignored?
There was a problem hiding this comment.
In practice this is only a violation if invokeMapMethod is violating the lint, since we're just rethrowing the same object. That seemed unlikely enough that I didn't think it was worth worrying about given that the plan is to migrate all plugins to Pigeon over time, and this code would be eliminated in a Pigeon migration.
|
The LUCI failures are infra problems. I do need to fix the excerpts though. |
|
It looks like some of the excerpts failures are false positives; I'll do a quick fix to flutter/flutter#111592 and then revisit this. |
| // the switch rather than a `default` so that the linter will flag the | ||
| // switch as needing an update. | ||
| // ignore: dead_code | ||
| return 'max'; |
There was a problem hiding this comment.
Do you think we should just keep the old behavior of throwing? It seems like in most of these cases you can keep the same behavior by removing the exception outside of the default branch which would keep the same logic.
There was a problem hiding this comment.
We could keep the same logic, but I changed it because throwing seems very extreme to me when there's a reasonable fallback.
When these switches were first written, everything was one package and this case meant that we'd forgotten to implement something, so it was a bug. Now it means that a client is using an implementation version that doesn't support that option yet, so a fallback seems better to me.
|
@tarrinneal or @bparrishMines could one of you take the review on this one? |
bparrishMines
left a comment
bparrishMines
left a comment
There was a problem hiding this comment.
LGTM with a question
| return Icons.camera; | ||
| // This enum is from a different package, so a new value could be added at | ||
| // any time. The example should keep working if that happens. | ||
| // ignore: no_default_cases |
stuartmorgan-g
added
the
autosubmit
* Change the way local changes are handled to match packages * only_throw_errors * Enable no_default_cases * Fix violations in camera * Fix violations in webview * Fix url_launcher violation * Fix violations in shared_preferences * Fix violations in maps * Version bumps * Fix image_picker violations * Fix video_player violations * New version bumps * Update excerpts * Address review feedback
5 participants
Small updates to analysis_options.yaml to better align with flutter/packages (and thus flutter/flutter, since it has more flutter/flutter options enabled than flutter/plugins currently does):
no_default_casesonly_throw_errorsno_default_casesrequired the most changes. I wrote up the approach I used at https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#enum-handling since we should have a consistent pattern across all plugins. I put an inline explanation for eachignorethough so that it doesn't require jumping to a browser to understand it, and won't become mysterious if that link breaks someday.Part of flutter/flutter#76229
Part of flutter/flutter#113764
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.///).