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

[path_provider] Log errors in the linux example #3146

Merged
merged 6 commits into from
Nov 19, 2020

Conversation

jiahaog
Copy link
Member

@jiahaog jiahaog commented Oct 13, 2020

When exceptions happen in the linux example, they are silently dropped and do not show up in the logs (flutter/flutter#66593).

This PR prints them out to the console so that we can understand what went wrong.

When exceptions happen during flakes, they are silently dropped and do not show up in the logs (flutter/flutter#66593).
@google-cla google-cla bot added the cla: yes label Oct 13, 2020
@jiahaog jiahaog marked this pull request as ready for review October 16, 2020 08:35
@jiahaog
Copy link
Member Author

jiahaog commented Oct 21, 2020

@TimWhiting can you please take a look at this :)

@TimWhiting
Copy link
Contributor

Hi @jiahaog
I'm not a member of the flutter team, but did help with the linux implementation and example for this plugin.
I would defer to them for what they want to do with this.

The reason why I implemented it this way was because if one call throws then the rest won't execute leaving the other strings as null. I'm pretty sure it is best practice to catch errors rather than letting them crash leaving null scattered through the app. And we should encourage plugin users to catch errors so they don't run into problems like I mentioned with not initializing something because an error was thrown before it got to that point. However, instead of just assigning an error string in the catch block we could catch each error and then rethrow the last error at the end of initialization if there was an error. But I'm not sure the best way to handle this.

@jiahaog
Copy link
Member Author

jiahaog commented Oct 28, 2020

Thanks, that makes sense. I think to keep things simple we could also print the error to the console instead of just dropping it, I will send an update for this 🙂

@jiahaog jiahaog changed the title [path_provider] Don't drop errors in the example [path_provider] Log errors in the example Nov 6, 2020
@jiahaog
Copy link
Member Author

jiahaog commented Nov 6, 2020

@stuartmorgan can you help to take a look at this?

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.

Code changes seem fine, but please fix the title and/or description to make it clear this is about Linux specifically.

I'm also not sure what the repo policy on changing just examples is; this may need a version and changelog update?

@jiahaog jiahaog changed the title [path_provider] Log errors in the example [path_provider] Log errors in the linux example Nov 10, 2020
@jiahaog
Copy link
Member Author

jiahaog commented Nov 10, 2020

Updated the description. I don't think this will require a version update - if I understand correctly the example code is not shipped to users on pub. What do you think?

@stuartmorgan-g
Copy link
Contributor

What do you think?

As I said, I don't know what the repo policy on that is. Someone more familiar with flutter/plugins will need to weigh in.

@jiahaog
Copy link
Member Author

jiahaog commented Nov 12, 2020

Checked with @cyanglaz and we need to update the version and changelog, so did that.

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

@jiahaog
Copy link
Member Author

jiahaog commented Nov 19, 2020

Thanks!

@jiahaog jiahaog merged commit e4a0642 into flutter:master Nov 19, 2020
@jiahaog jiahaog deleted the path-provider-rethrow branch November 19, 2020 23:32
amantoux pushed a commit to amantoux/plugins that referenced this pull request Feb 8, 2021
adsonpleal pushed a commit to nubank/plugins that referenced this pull request Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants