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

Add implements value to the pubspec #3577

Closed
wants to merge 21 commits into from

Conversation

blasten
Copy link

@blasten blasten commented Feb 19, 2021

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM

@blasten
Copy link
Author

blasten commented Feb 19, 2021

@stuartmorgan I added some TODOs. would you mind checking that this is what you expect? (I'm not familiar with the Desktop plugins)

@cyanglaz
Copy link
Contributor

These could also be part of stable releases for those plugin to avoid 1 additional publish.

@@ -1,3 +1,8 @@
## 2.0.0-nullsafety.1
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to do an actual point release for this since they already landed.

Copy link
Author

Choose a reason for hiding this comment

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

done

@blasten blasten requested a review from stuartmorgan February 20, 2021 00:35
@blasten
Copy link
Author

blasten commented Feb 20, 2021

@stuartmorgan PTAL

@blasten blasten requested a review from stuartmorgan February 20, 2021 00:58
@blasten
Copy link
Author

blasten commented Feb 20, 2021

Due to transitive dependencies, I would need to merge this PR on red, and republish the packages, bump dependencies, and rerun the checks.


flutter:
plugin:
implements: package_info
Copy link
Contributor

Choose a reason for hiding this comment

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

package_info implements package_info? I thought implements was only for federated plugins.

Copy link
Author

Choose a reason for hiding this comment

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

default_package is more appropriate here. The schema validator found macos platform, but no implements:, nor default_package: value. Done

@stuartmorgan

This comment has been minimized.

@blasten blasten requested a review from stuartmorgan February 20, 2021 04:02
@blasten blasten force-pushed the add_implements branch 7 times, most recently from 2de65de to 9386423 Compare February 24, 2021 17:53
@blasten
Copy link
Author

blasten commented Feb 24, 2021

This PR is blocked until the tools changes land in stable.

@stuartmorgan
Copy link
Contributor

Do you want to break this apart into a couple of PRs? You could just add implements and registerWith everywhere now, which should be inert for now, and then do a later PR to unwind all the manual registration once the full support reaches stable.

@blasten
Copy link
Author

blasten commented Mar 1, 2021

sgtm

@blasten
Copy link
Author

blasten commented Apr 28, 2021

@stuartmorgan This PR won't be able to land until the next stable because it would break users using these plugins and Flutter stable.

@stuartmorgan
Copy link
Contributor

What's changed since the discussion above (#3577 (comment)) about being able to land part of this now?

@blasten
Copy link
Author

blasten commented Apr 28, 2021

Primarily, I was looking at the shim https://github.com/flutter/plugins/pull/3577/files#diff-0a54019786f62bf41f0c6d1481397f36d46d3a18abbead0d0467d2e0acdeeb1dL30, and thought it would conflict, but now I realize that it checks if SharedPreferencesStorePlatform.instance has already been set.

Sgtm

@stuartmorgan
Copy link
Contributor

Now that 2.5 has shipped I believe this could be updated (implements has been added, but the registration workarounds still need to be removed) and landed.

@stuartmorgan
Copy link
Contributor

@blasten Do you want to merge in master here and see if this is ready to land?

@stuartmorgan
Copy link
Contributor

I did the registration removal in separate PRs (one landed, one outstanding) so this is obsolete.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building with a macOS plugin: "Plugin url_launcher_linux doesn't implement a plugin interface"
3 participants