Skip to content

Improve error handling#86

Open
JSpiner wants to merge 7 commits intoParkSangGwon:masterfrom
JSpiner:master
Open

Improve error handling#86
JSpiner wants to merge 7 commits intoParkSangGwon:masterfrom
JSpiner:master

Conversation

@JSpiner
Copy link

@JSpiner JSpiner commented Feb 19, 2018

related with #81

  1. Add defaultErrorListener
    I created a default listener that only throws an OnErrorNotImplementedException if it is not set.

  2. Add error handling
    Added error handling at empty handling code.

@ParkSangGwon
Copy link
Owner

Do you want change policy forced to implement ErrorListener?
As-is policy is setOnErrorListener() is optional

@JSpiner
Copy link
Author

JSpiner commented Feb 20, 2018

@ParkSangGwon
No! It does not force to implement ErrorListener.
As we discussed,

Library단에서 OnErrorListener를 set()해주도록 제공해주고 만약 구현되어있다면 해당 listener에게 알려주고
그렇지 않다면 throw로 에러를 발생시키는 방식으로 하면 좋을것 같습니다.

If ErrorListener is implemented, just call it.
If ErrorListener is not implemented and error occurs, it throws error by default listener.

@JSpiner
Copy link
Author

JSpiner commented Feb 20, 2018

OnErrorNotImplementedException is thrown if an error has occurred and an ErrorListener has not been implemented.

@ParkSangGwon
Copy link
Owner

@JSpiner I forgot...
Then please add setOnErrorListener() in sample code

@JSpiner
Copy link
Author

JSpiner commented Feb 20, 2018

That's good.
I'll add some sample code and push changes!
Thanks.

@ParkSangGwon
Copy link
Owner

@JSpiner
Please add sample code.

@JSpiner
Copy link
Author

JSpiner commented Dec 28, 2018

@ParkSangGwon
can you review it?

@ParkSangGwon
Copy link
Owner

@JSpiner
Sorry, I can not remember this comment.

Library단에서 OnErrorListener를 set()해주도록 제공해주고 만약 구현되어있다면 해당 listener에게 알려주고
그렇지 않다면 throw로 에러를 발생시키는 방식으로 하면 좋을것 같습니다.

Please make optional setOnErrorListener().
This mean even If user didn't implement error listener, error don't occur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants