Skip to content
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

Fix: Proper error handling #81

Closed
pouya-eghbali opened this issue Mar 23, 2024 · 6 comments
Closed

Fix: Proper error handling #81

pouya-eghbali opened this issue Mar 23, 2024 · 6 comments

Comments

@pouya-eghbali
Copy link
Member

There are a lot of places in the code where we just panc(err). That shouldn't be the case; we need to handle these errors properly.

@logicalangel
Copy link
Contributor

We know that all errors should not close our app with os error. There are two approaches to dealing with errors:

first, top-down:

  • log errors when they happen, it gives the ability to have the source of them, and after that return a customized internal error (it is better to aggregate all custom errors in one place)
if err != nil {
   log.Error(err)
   return errors.New("something is wrong")
}

second way, down-top:
return all errors by wrapping them with some extra information with errors.wrap(err, "some extra data"). and when they come to the nearest layer to the endpoint, convert them to customized errors or show them when we are in development mode.

I suggest the first one.

@pouya-eghbali
Copy link
Member Author

Let's go with the top-down approach.

@pouya-eghbali
Copy link
Member Author

@harpy-wings this should be done after merging #84, you should branch out from #84. You'll have to rebase after the merge if you branch out from develop.

@harpy-wings
Copy link

we should use Error Wrapping,

err := doSomething()
if err != nil {
  return errors.Join(err,errors.New("unexpected failure))
}

note that it may not be required to use the same thing everywhere. sometimes a simple return is better.
we should wrap errors while we are changing the layers, for example from the service layer to application gateways it's better to be wrapped.

@pouya-eghbali pouya-eghbali changed the title Fix: proper error handling Fix: Proper error handling Mar 25, 2024
@harpy-wings
Copy link

also, there is a dependency on refactoring since an error should be returned instead of panic or log. and we need to change the function's interface for all of the packages.
@logicalangel let me know what you think and how we should approach this.

@logicalangel
Copy link
Contributor

duo to #107 all panic are replaced, but we should consider error handling in the next PRs.

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

No branches or pull requests

3 participants