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

[path_provider] Create platform interface #2553

Merged
franciscojma86 merged 17 commits into from
Feb 26, 2020
Merged

[path_provider] Create platform interface #2553

franciscojma86 merged 17 commits into from
Feb 26, 2020

Conversation

@franciscojma86
Copy link
Contributor

@franciscojma86 franciscojma86 commented Feb 25, 2020 •
edited
Loading

Description

Creates a platform_interface for path_provider. Second step for federation outlined in flutter/flutter#46307 (comment)

Related Issues

flutter/flutter#46307

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.

ditman
ditman approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Other than the philosophical question about dart:io, this looks good!

ditman
ditman approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Some small tweaks that you can do here and there. Still not a fan of doing a platform check to throw an exception before calling the method channel. The native implementation should return an unimplemented exception (I'm sure it does already).

...ges/path_provider/path_provider_platform_interface/lib/src/method_channel_path_provider.dart
Comment on lines +85 to +87
if (!_platform.isMacOS) {
throw UnsupportedError('Functionality only available on macOS');
}
Copy link
Member

@ditman ditman Feb 26, 2020

Choose a reason for hiding this comment

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

Same comment I did in your previous platform interface migration. IMO this should be thrown by the Java side (I don't know what to do with the 'getDownloadsDirectory' call, and not in the MethodChannel implementation).

If the Fuchsia version is able to fulfill this, this'd be throwing an exception before it even reaches native code.

Copy link
Contributor Author

@franciscojma86 franciscojma86 Feb 26, 2020

Choose a reason for hiding this comment

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

Not sure I follow. I'm keeping it here to make sure it's as similar to the current implementation as possible. I think you are right about the fact that the native side would just return MissingImplementation. I think if we want that exception as opposed to the UnsupportedError that might be a bigger discussion to be consistent with the rest of the plugins.

What do you mean by the fuchsia version example? In theory, nothing other than macOS should reach the native code. Maybe that's why the platform checks were added? To save some calls if we know they are going to fail? Also, maybe for testing :)

Copy link
Member

@ditman ditman Feb 26, 2020

Choose a reason for hiding this comment

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

Let's agree to disagree :P

Copy link
Contributor Author

@franciscojma86 franciscojma86 Feb 26, 2020

Choose a reason for hiding this comment

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

Oh I don't have a strong posture on this at all :) It's mostly a "Want to change as little as possible from the way it already was". I think @amirh or @cyanglaz might have a better opinion on this.

@franciscojma86 franciscojma86 merged commit 458b524 into flutter:master Feb 26, 2020
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
@franciscojma86 @EdwinRomelta
[path_provider] Create platform interface (flutter#2553)
edc1498
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@franciscojma86
[path_provider] Create platform interface (flutter#2553)
fb5d5a3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@amirh amirh Awaiting requested review from amirh

1 more reviewer

@ditman ditman ditman approved these changes

Reviewers whose approvals may not affect merge requirements

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

@franciscojma86 @ditman @googlebot