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

Conversation

@joaomagfreitas
Copy link
Contributor

Description

Updated pickImage and pickVideo docs to expose the possible errors that can be thrown as a PlatformException. The reason I want the docs to be updated with this information is due to developers not being aware of the errors that can be thrown when reading the plugin documentation.

Related Issues

No related issues found.

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.

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.

Thanks for doing this! I left some comments!

MethodChannel('plugins.flutter.io/image_picker');

/// Returns a [File] object pointing to the image that was picked.
/// Returns a [File] object pointing to the image that was picked or throws
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

Returns a [File] object pointing to the image that was picked.
Throws ...

Also, the message might be a little too definitive as there might be other platform exception thrown due to some error caused in the platform. (Like some iOS errors unknown to us), we might want to re-word it a little. Maybe something like "The method could throw a [PlatformException] if..."

Copy link
Contributor Author

@joaomagfreitas joaomagfreitas Apr 21, 2020

Choose a reason for hiding this comment

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

Hi @cyanglaz

Thanks for the feedback! I update the docs to explicit that the method could throw a PlatformException but left the description of the possible exceptions, because I think it is necessary for developers to be aware of the common exceptions that they might have to deal with when reading the documentation. If on the method docs is not the best place, could it be explicit instead on README.md?

Thanks,
freitzzz

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't make myself clear enough. I meant what you have done is mostly ok, just that we need to indicate there might be other errors besides the ones that you have there.

Copy link
Contributor Author

@joaomagfreitas joaomagfreitas Apr 21, 2020

Choose a reason for hiding this comment

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

Sorry I didn't make myself clear enough. I meant what you have done is mostly ok, just that we need to indicate there might be other errors besides the ones that you have there.

Got it! Just updated to reference that PlatformException could also be thrown due to an unknow error. Is it ok for now, or do you think it needs to be rephrased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyanglaz Let me know if you still want these changes

Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely! If you could rebase and resolve the conflicts, I will land this! Thanks again.

@cyanglaz cyanglaz self-assigned this Apr 20, 2020
@stuartmorgan-g
Copy link
Contributor

It looks like this review got lost. If you could update to resolve the code conflicts and then let me know when it's ready, I can make sure it gets a final review.

@joaomagfreitas
Copy link
Contributor Author

It looks like this review got lost. If you could update to resolve the code conflicts and then let me know when it's ready, I can make sure it gets a final review.

Hi @stuartmorgan

Yes, it sure did. Sorry for the inconvenience, but I totally forgot about it.
Currently resolving the conflicts, will ping you asap

@joaomagfreitas
Copy link
Contributor Author

@stuartmorgan @cyanglaz

I noticed that I had deleted my previous fork of the plugins repo, so I had to create a new PR. Will close this one and ref the new one

#4089

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.

4 participants