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

ignore upcoming warnings #5852

Closed
wants to merge 3 commits into from
Closed

Conversation

a14n
Copy link
Contributor

@a14n a14n commented May 26, 2022

This PR will prevent warnings once flutter/flutter#104231 has landed.

@github-actions github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-ios labels May 26, 2022
@a14n a14n requested a review from goderbauer May 26, 2022 21:04
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

Can you also add a TODO above these that says to remove the imports once flutter/flutter#104231 lands?

@a14n a14n force-pushed the ignore-upcoming-warnings branch from 94954f7 to 37af759 Compare May 27, 2022 07:17
@a14n
Copy link
Contributor Author

a14n commented May 27, 2022

Can you also add a TODO above these that says to remove the imports once flutter/flutter#104231 lands?

Done.

I also added a No version/CHANGELOG change to make publishable task happy. Please let me know if it's the good solution.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I also added a No version/CHANGELOG change to make publishable task happy. Please let me know if it's the good solution.

No, these need a version change because they don't meet our exemption criteria. These are warnings that will show up for clients of the plugin.

@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// TODO(a14n): remove this import once flutter/flutter#104231 lands
Copy link
Contributor

Choose a reason for hiding this comment

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

These TODOs aren't actually accurate; we need to remove the import when that PR reaches stable, because all of our plugins support the stable channel. It should say something like "once Flutter 3.1 or later reaches stable".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. But given that this PR hasn't even reached master we should perhaps prefer once flutter/flutter#104231 reaches stable.

@a14n
Copy link
Contributor Author

a14n commented May 30, 2022

No, these need a version change because they don't meet our exemption criteria.

Done. Thanks for your help.

These are warnings that will show up for clients of the plugin.

I'm not sure to understand what you refer to. Could you give me an example please?

@a14n a14n force-pushed the ignore-upcoming-warnings branch from 6e4e7ab to 16892df Compare May 30, 2022 06:36
@a14n
Copy link
Contributor Author

a14n commented May 30, 2022

According to the federated_safety task I splitted this PR into #5855 and #5856 .

@a14n a14n closed this May 30, 2022
@a14n a14n deleted the ignore-upcoming-warnings branch May 30, 2022 07:03
@stuartmorgan
Copy link
Contributor

These are warnings that will show up for clients of the plugin.

I'm not sure to understand what you refer to. Could you give me an example please?

  1. Make a new Flutter app.
  2. Add this lint.
  3. Add a dependency on webview_flutter
  4. Build using a local version of Flutter with add some exports of public API in foundation/serialization.dart flutter#104231

You will see these warnings; the same would happen for everyone else once that PR has landed.

@a14n
Copy link
Contributor Author

a14n commented May 31, 2022

The client will see (new) warnings in its own code but he will not see warnings inside the webview_flutter plugin he depends on. That's why I didn't thought a Changelog entry was needed (before reading the exemption criteria).

Thanks for your feedback!

@stuartmorgan
Copy link
Contributor

The client will see (new) warnings in its own code but he will not see warnings inside the webview_flutter plugin he depends on.

Why wouldn't they see warnings from webview_flutter? I thought all transitive Dart warnings were surfaced.

@a14n
Copy link
Contributor Author

a14n commented May 31, 2022

Why wouldn't they see warnings from webview_flutter? I thought all transitive Dart warnings were surfaced.

I just made a quick test and it looks like the analyzer don't show any warning from the dependency.

@goderbauer
Copy link
Member

Why wouldn't they see warnings from webview_flutter? I thought all transitive Dart warnings were surfaced.

No, in your app or package you will not see warnings that occur inside your dependencies.

@stuartmorgan
Copy link
Contributor

stuartmorgan commented May 31, 2022

Oh, sorry about that. I must have just conflated the Dart behavior with the native plugin builds that I'm more used to.

In that case you were correct that these don't need version changes (although should still have changelog changes).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p: webview_flutter Edits files for a webview_flutter plugin platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants