New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ErrorHandler more robust #1466 #1578

Closed
wants to merge 1 commit into
base: development
from

Conversation

Projects
None yet
2 participants
@sebd71
Copy link

sebd71 commented Feb 7, 2019

Don't expose internal ErrorHandlers map and declare Get/Set methods to manage registered ErrorHandlers.

  • make ErrorHandlers Get function more robust (handle nil)
  • introduce Clear, Clone, Set and SetMulti functions
  • export DefaultErrorHandler function so that user can call it from its
    handler if needed be
  • add a new test to check Clear and SetMulti functions
Make ErrorHandler more robust #1466
Don't expose internal ErrorHandlers map and declare Get/Set methods to
manage registered ErrorHandlers.

- make ErrorHandlers Get function more robust (handle nil)
- introduce Clear, Clone, Set and SetMulti functions
- export DefaultErrorHandler function so that user can call it from its
  handler if needed be
- add a new test to check Clear and SetMulti functions

Change-Id: Ida701f6c6dea202db788f1dd805e2b7ccdff14cb
Signed-off-by: Sebastien Douheret <sebastien.douheret@iot.bzh>

@sebd71 sebd71 requested a review from gobuffalo/core-managers as a code owner Feb 7, 2019

@fixmie
Copy link

fixmie bot left a comment

Found some fixes!

P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).

*/
type ErrorHandlers map[int]ErrorHandler

type ErrorHandlersST struct {

This comment has been minimized.

@fixmie

fixmie bot Feb 7, 2019

exported identifier "ErrorHandlersST" should have comment

Suggested change Beta
type ErrorHandlersST struct {
// ErrorHandlersST ...
type ErrorHandlersST struct {
@markbates

This comment has been minimized.

Copy link
Member

markbates commented Feb 10, 2019

Thanks for the feedback. I’m not convinced this needs the full rewrite you propose. The first issue can be fixed with a simple nil check. The second one is expected behavior. If I replace the whole ErrorHandlers with a new one, I would expect it to replace any previous changes I made to it.

I should also point out that you’re requesting breaking changes. I would recommend you open a proposal ticket and lay out your ideas there so the community can discuss the pros/cons of these changes.

@markbates markbates closed this Feb 10, 2019

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