8000 [path_provider] linux endorsement by TimWhiting · Pull Request #2789 · flutter/plugins · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[path_provider] linux endorsement #2789

Merged
merged 14 commits into from
Jun 2, 2020
Merged

[path_provider] linux endorsement #2789

merged 14 commits into from
Jun 2, 2020

Conversation

TimWhiting
Copy link
Contributor

Description

This PR updates path_provider to endorse the dart Linux path provider manually, until automatic registration of dart plugins are working.

Related Issues

flutter/flutter#41716

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?
Only to remove manual registration if anyone does that before this gets implemented.

  • 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.

@TimWhiting
Copy link
Contributor Author
TimWhiting commented May 21, 2020 •
edited
Loading

@stuartmorgan
FYI Here is the PR for the endorsement of the Linux path provider implementation.

Note that I need to manually register the instance, at least until flutter tool support for automatically generating a plugin registrant for dart only plugins happens. If that is in the plans? (I didn't see any conclusion on which approach you are planning on taking) Design Document: Flutter tool support for federated plugins

@stuartmorgan-g
Copy link
Contributor

Note that I need to manually register the instance, at least until flutter tool support for automatically generating a plugin registrant for dart only plugins happens. If that is in the plans?

Ideally yes, but it ended up having technical complexities, so we decided to go with this approach in the short-to-medium term to unblock being able to implement Dart-only plugins. flutter/flutter#52267 is tracking a longer term solution.

TimWhiting added 4 commits May 22, 2020 08:44
@TimWhiting
cleaned up the implementation slightly to be more readable and reflec…
b4c61a7 
…t the suggestions for manual registrations in the design document, also bumped the linux path provider version since updated readme and example to reflect the endorsement
@TimWhiting
merged in master
d213113
@TimWhiting
fixed return statement
0cd2b1f
@TimWhiting
increase timeout for flakey test
8924386
@TimWhiting
Copy link
Contributor Author

@stuartmorgan
Ready for review

stuartmorgan-g
stuartmorgan-g suggested changes May 26, 2020
View reviewed ch 8000 anges
@TimWhiting
Copy link
Contributor Author

@stuartmorgan
Just made that setter visible for testing, and updated the dartdoc for it. Let me know if further adjustments should be made. I tried to not change the dartdoc too much, in case we want to go back to being user visible.

I think this should be ready to land, once all the tests pass.

stuartmorgan-g
stuartmorgan-g approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor
@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

LGTM!

8000
@TimWhiting
Copy link
Contributor Author
TimWhiting commented Jun 2, 2020 •
edited
Loading

@stuartmorgan
Do I need to merge in master with that latest merged pull request or can you merge this in?

@stuartmorgan-g
Copy link
Contributor

There aren't an conflicts; no need to re-merge.

@stuartmorgan-g stuartmorgan-g merged commit fc6bd98 into flutter:master Jun 2, 2020
@stuartmorgan-g
Copy link
Contributor

Once main CI has cycled green I should be able to publish the update.

@stuartmorgan-g
Copy link
Contributor

path_provider 1.6.10 is live 🎉

@TimWhiting TimWhiting deleted the path_provider_endorse_linux branch June 3, 2020 17:58
@TimWhiting TimWhiting mentioned this pull request Jun 4, 2020
Merged
12 tasks
@TimWhiting
Copy link
Contributor Author
TimWhiting commented Jun 4, 2020 •
edited
Loading

@stuartmorgan
I think this was unintentionally a breaking change.
It doesn't break runtime behavior (it only helps Linux for that).
However, it does break test behavior if the test is run on Linux and is mocking the MethodChannel, and not the PathProviderPlatform.
See #2589 (comment). The comment isn't exactly clear about the issue, but I'm pretty sure it was an issue related to the root cause I explained above.

Ideally people migrating to the federated plugin, should have also updated their mocks of the plugin, but this might not have happened. Especially since the documentation on writing tests has this example of mocking the MethodChannel of path_provider for testing. https://flutter.dev/docs/cookbook/persistence/reading-writing-files#testing

What should we do?
In the current state, we would want to update the documentation of path_provider to highlight the changes that need to be made to tests (either update the mock to mock the PathProviderPlatform, or set disablePathProviderPlatformOverride = true), as well as update the cookbook.

@stuartmorgan-g stuartmorgan-g mentioned this pull request Mar 24, 2023
Closed
@stuartmorgan-g
Copy link
Contributor

I filed flutter/website#8442 for updating the cookbook. Updating the path_provider docs to call out the necessary changes sounds good. Given that it's already published, I don't think there's much more we can do to mitigate the potential unintentional test breakage.

@TimWhiting TimWhiting mentioned this pull request Jun 4, 2020
Merged
13 tasks
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
@TimWhiting @EdwinRomelta
[path_provider] linux endorsement (flutter#2789)
83f1d35 
Endorses path_provider_linux, manually registering its implementation as a workaround for flutter/flutter#52267
@Sunbreak Sunbreak mentioned this pull request Jun 12, 2020
Merged
13 tasks
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
@TimWhiting @jorgefspereira
[path_provider] linux endorsement (flutter#2789)
40d2242 
Endorses path_provider_linux, manually registering its implementation as a workaround for flutter/flutter#52267
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@TimWhiting
[path_provider] linux endorsement (flutter#2789)
feec2c4 
Endorses path_provider_linux, manually registering its i
6BB2
mplementation as a workaround for flutter/flutter#52267
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TimWhiting @stuartmorgan-g @googlebot
0