-
Notifications
You must be signed in to change notification settings - Fork 13
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
Manual notify exception #76
base: master
Are you sure you want to change the base?
Conversation
I think we can send (or teach the user to send) the stacktrace from the rescue block, using STACKTRACE/0. |
I think it is fair to have a unique global configuration, so we have no other choice :) |
|
||
```elixir | ||
defmodule MyController do | ||
use BoomNotifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be an import
instead of a use
. So, you can avoid all the extra complexity of checking if we are in the context of a router or a controller in BoomNotifier.__using__
.
In fact, it is probably fine to say that the user should invoke BoomNotifier.notify_error
directly (the user can or not add an import
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the only thing to solve here is to make sure that config validations runs at least once. If the module is used in the Router is fine, but we should make it mandatory (even if it is not going to be used there).
Another option would be to check configs directly in the new Config
module, as part of any of the functions defined there.. or maybe as an operation that is done only once and cached.
do_notify_error(conn, error) | ||
end | ||
|
||
def manual_notify_error(conn, error) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the already existing notify_error
function? I think all the logic there applies here (limits & filters for exceptions). So, we would have similar ways in router and controller to notify errors when exceptions are being intercepted by user code (error_handler
or simple try-catch-rescue
blocks).
This PR aims to implement the ability to manually send an exception. It's a first try so feel free to discuss / suggest whatever you want. Actually, there are a few things to discuss to set a direction to go.
To do / discuss
handle_errors
. I don't like it too much but I think it covers most of the cases (it doesn't cover if the user defines a module ending in.Router
)use BoomNotifier
anywhere and with the current implementation it would be necessary to always write itconn
object or do we want to be more open to any elixir code and just send the exception without much more information?manual_notify_error
callback, but maybe it is better to have only onenotify_error
that handles all cases?