-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[url_launcher] migrate url_launcher package to null safety #3148
Conversation
f77d6db
to
b975b07
Compare
Let me know when this is ready to review |
@blasten, it would be great if you can do a review on the code. I think everything looks good, I am just waiting on a reply from David regarding some information why the integration tests won't run (I think this is related to the changes in #3158 which adds nnbd support in the integration_test package). |
27d3835
to
dc9a87a
Compare
@mvanbeusekom would you mind rebasing your branch? |
@@ -1,3 +1,7 @@ | |||
## 5.8.0-nullsafety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not 6.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the version number in the middle of the discussion we had for the url_launcher_platform_interface
. Since for that plugin we settled on doing a major release I think it should be good to do it here as well.
So I updated the version to 6.0.0-nullsafety. Main argument the canLaunch
and launch
methods won't be returning an optional null
value anymore (where in earlier versions this was a possibility).
include: ../../../analysis_options.yaml | ||
analyzer: | ||
enable-experiment: | ||
- non-nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end of file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -1,3 +1,7 @@ | |||
# 0.2.0-nullsafety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to do web as well? cc @ditman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thankful this is migrated, however integration_tests aren't currently running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
circling back, you guys discussed offline to revert url_launcher_web
. I'm fine with that. Thanks for the effort @mvanbeusekom!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ditman Maybe I can revert it back in this PR and open a second one just for the url_launcher_web package as draft and have it ready to go when all dependencies get fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mvanbeusekom that sounds great, that way all the effort doesn't go to waste! :)
b513bd2
to
138a960
Compare
Thanks for the review, I rebased the branch and also processed your other feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
BREAKING CHANGE: this will introduce null safety and thus updates the signature of several methods.
This reverts commit b199e62.
83d2035
to
42719e7
Compare
Description
This pull request add null safety support to the url_launcher package.
The implementation depends on companion PR #3142 which add null safety support to the url_launcher_platform_interface package. Since PR #3142 isn't merged as of yet the dependency is currently based on a path reference, which should be fixed before merging this PR.
Related Issues
flutter/flutter#66395
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?