[android_intent] Bump to Flutter stable, remove deprecation warnings #2586
Conversation
| sdk: ">=2.0.0-dev.28.0 <3.0.0" | ||
| flutter: ">=1.10.0 <2.0.0" | ||
| sdk: ">=2.3.0 <3.0.0" | ||
| flutter: ">=1.12.13+hotfix.5 <2.0.0" |
There was a problem hiding this comment.
Is there a reason that we're bumping the min supported Dart and Flutter versions?
There was a problem hiding this comment.
The Dart version was increased to make the spread operator available.
Flutter version was increased to the latest stable version to be inline with other plugins like google_maps, connectivity etc.
There was a problem hiding this comment.
Should we be forcing all consumers of this plugin to upgrade their Dart version just so that this plugin can use the spread operator? I'm not sure how many people are on various versions of Dart, but my initial thought is that using the spread operator is a pretty trivial detail and we probably shouldn't force a language change on people just do that. Thoughts?
There was a problem hiding this comment.
Glad you asked. I had similar concerns but ended up with:
- Current stable is referring to Dart 2.7, while this PR only bumps to 2.3 (when spread was introduced, roughly)
- Every time I bump a Flutter version on my machine, the bundled Dart version is synced
- The previous stable version (1.9) shipped with Dart 2.5, so I think we're ok
- IMO Flutters official plugins should showcase new language (stable) features so that users adopt them quicker, thus making the the platform more attractive
- Flutters style guide mentions how to use the spread operator, so I assumed it's already accepted
- Reducing LOC is always a compelling argument although not too strong one here
There was a problem hiding this comment.
I think current stable including a higher version of Dart already makes this a non-issue.
There was a problem hiding this comment.
As @mklim mentioned, your first bullet point is probably the right answer. If stable has bumped Dart then we're probably good.
I would caution the 2nd and 3rd bullet points. I actually don't think using new language features or showcasing capabilities would be seen as an appropriate reason to break versions. I'll defer to @amirh and @Hixie in that regard, but that would be my guess.
In any case, bullet point 1 makes sense and works for me.
There was a problem hiding this comment.
LGTM. This PR doesn't introduce tests, but the existing tests in android_intent already cover its functionality since this is a refactor with no behavioral changes.
If you're open to expanding this PR more, there's a longer list of TODOs we'd like to fix on each plugin when we upgrade it to the latest stable (flutter/flutter#47153). There are a few hacks in this plugin to make it backwards compatible with the previous stable that aren't needed since you're bumping the version now, and they could be fixed as part of this PR. I don't think this PR should be blocked on those TODOs though so no pressure if you don't want to expand this more and just want to land this as-is.
| sdk: ">=2.0.0-dev.28.0 <3.0.0" | ||
| flutter: ">=1.10.0 <2.0.0" | ||
| sdk: ">=2.3.0 <3.0.0" | ||
| flutter: ">=1.12.13+hotfix.5 <2.0.0" |
There was a problem hiding this comment.
I think current stable including a higher version of Dart already makes this a non-issue.
|
@mklim @matthew-carroll thank you for reviewing this PR. How could the updated packaged be pushed to pub? |
…lutter#2586) * Bumps Flutter & Dart dependencies to stable * Uses spread operator to concisely build arguments * Refactors and cleans up the plugin a bit
…lutter#2586) * Bumps Flutter & Dart dependencies to stable * Uses spread operator to concisely build arguments * Refactors and cleans up the plugin a bit
Description
Bumps dart & flutter version & adds a little bit of house keeping so that the next PR is distraction free.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?