-
Notifications
You must be signed in to change notification settings - Fork 28
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
Notes & Questions #5
Comments
This probably should depend on the specific error type? If the response is >=500, I'd think logging is appropriate. If it's something like a 400 bad request that we have gotten from our request validator, then logging is probably not necessary. |
Would follow the lead of some other projects and have this as a configurable option: Graphiti::Rails.configure do |c|
c.base_controller = 'ApplicationController'
end |
The list of errors you pulled from graphiti to merge into That has a custom error handler subclass in graphti_errors: https://github.com/graphiti-api/graphiti_errors/blob/master/lib/graphiti_errors.rb#L23-L26 |
I wrote my own notes here: https://gist.github.com/richmolj/ded6c99353ead759785caaeeb68a7e60 To touch on a few things Wade mentioned:
I think these are good defaults, we just need the ability to override with
I'd prefer including a mixin as a little more explicit. The downside to applying to all controllers is running AMS/whatever and graphiti side-by-side. |
@wadetandy for now, I think we'll only log fatal exceptions. See eeb6b6d. |
See also #7 |
For easier discussion, I've split these up into multiple issues. If I missed anything, please open a new issue. |
Wanted a place to capture random thoughts while looking over the code and also a place to address questions you ask in your comments.
The text was updated successfully, but these errors were encountered: