FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(Android): better Async error handling by NathanWalker · Pull Request #147 · NativeScript/tns-core-modules-widgets · GitHub

This repository was archived by the owner on Mar 21, 2019. It is now read-only.

fix(Android): better Async error handling - #147

Merged
vchimev merged 4 commits into
NativeScript:masterfrom
nstudio:feature/async-error-handling
Nov 28, 2018
Merged

vchimev merged 4 commits into
NativeScript:masterfrom
nstudio:feature/async-error-handling

Conversation

Copy link
Copy Markdown
Contributor

PR Checklist

What is the current behavior?

Android can become unstable due to improper error handling around async requests.

What is the new behavior?

Better error condition handling which allow core modules to handle more appropriately.

Copy link
Copy Markdown
Contributor Author

The Async class also is the only one to make exclusive calls to e.printStackTrace however I would argue that should not be used and instead a standard log as described here:
https://stackoverflow.com/questions/3855187/is-it-a-bad-idea-to-use-printstacktrace-in-android-exceptions

I can modify to use Log per core team suggestion so leaving commented in these changes for now.

vchimev commented Nov 23, 2018

Copy link
Copy Markdown
Contributor

Hey @NathanWalker,

Could you please replace e.pinrtStackTrace() with Log and clean up the comments and formatting in order to proceed with merging?

Thank you!

vchimev self-assigned this Nov 26, 2018
NathanWalker force-pushed the feature/async-error-handling branch from eb56181 to cb4ed5e Compare November 26, 2018 19:14

Copy link
Copy Markdown
Contributor Author

@vchimev I have updated to use Log now and formatted - not sure why it shows entire file formatted though. Looks like all other files are formatted using 4 spaces which this one is now as well. Was it not properly formatted on master originally using Tabs? Lemme know if something else I need to do with formatting on this Async.java file but other than that should be good?

vchimev force-pushed the feature/async-error-handling branch from 5bbaf2d to edc3374 Compare November 28, 2018 08:15
vchimev force-pushed the feature/async-error-handling branch from edc3374 to 84ae894 Compare November 28, 2018 08:17

vchimev commented Nov 28, 2018
edited
Loading

Copy link
Copy Markdown
Contributor

Thank you, @NathanWalker!

I just replaced Log.v() with Log.e() and also checked out the formatting.

On its way to master.

vchimev merged commit ce45e75 into NativeScript:master Nov 28, 2018

Copy link
Copy Markdown
Contributor Author

Hooray @vchimev thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL