Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Migrate to null-safety #3310

Closed
wants to merge 1 commit into from
Closed

[webview_flutter] Migrate to null-safety #3310

wants to merge 1 commit into from

Conversation

mehmetf
Copy link
Contributor

@mehmetf mehmetf commented Dec 9, 2020

Description

Migrating webview-flutter to null-safety.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • [] No, this is not a breaking change.

@mehmetf mehmetf requested a review from amirh as a code owner December 9, 2020 01:00
@google-cla google-cla bot added the cla: yes label Dec 9, 2020
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some tests are failing and we have analysis errors

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 9, 2020

I have no idea why. I am possibly setting the dependency versions wrong. Could you (or someone else) help check?

@amirh
Copy link
Contributor

amirh commented Dec 9, 2020

The analysis unnecessary_null_comparison warnings seems legit.

The Gradle build errors on master channel are a framework regression, @blasten is handling that in flutter/flutter#71964.

The stable channel errors are because the plugin cannot work on stable after this change, as this is the first plugin we migrate on flutter/plugins master that's the first time we need to solve this. The easy fix would be to disable stable channel checks for the entire repository though I'm a little hesitant to do that, I'd prefer to only skip it for pre-release version that can't resolve on stable though I may be over-naive in how simple it is to do that. @blasten any thoughts on that? I can take a look at the CI change.

Side note - I basically have zero non-meeting time today so I only expect to get to it tomorrow.

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 9, 2020

Analysis errors are expected. They are coming from asserts. Is that lint turned off for package:flutter ?

@amirh
Copy link
Contributor

amirh commented Dec 9, 2020

Is the practice to keep assert(foo != null) for non nullable foo?

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 9, 2020

Yeah. That's what package:flutter has been following since these asserts have value for mixed mode code.

@amirh
Copy link
Contributor

amirh commented Dec 9, 2020

AFAICT unnecessary_null_comparison is not turned off in flutter/flutter (I'm not an expert of the analysis rules configuration for flutter/flutter though I believe https://github.com/flutter/flutter/blob/master/analysis_options.yaml is the source of truth)

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 9, 2020

@amirh
Copy link
Contributor

amirh commented Dec 9, 2020

AFAICT unnecessary_null_comparison is not turned off in flutter/flutter (I'm not an expert of the analysis rules configuration for flutter/flutter though I believe https://github.com/flutter/flutter/blob/master/analysis_options.yaml is the source of truth)

Actually that's not true, it's turned off here: https://github.com/flutter/flutter/blob/52ea6c2a318537eb4d2ab260143bccaf6ec2151a/packages/flutter/analysis_options.yaml#L8

I guess we should do the same then.

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 9, 2020

Cool.

@blasten
Copy link

blasten commented Dec 11, 2020

@mehmetf #3318 should help get the checks green on stable as it skips this plugin.

@mehmetf
Copy link
Contributor Author

mehmetf commented Dec 14, 2020

I am going to close this PR and open a new one.

@mehmetf mehmetf closed this Dec 14, 2020
@mehmetf mehmetf deleted the b005 branch December 14, 2020 18:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants