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
Replace white/blacklists with error handler #97
Conversation
SystemExit | ||
] | ||
raise error if classes.any? { |klass| error.is_a?(klass) } | ||
end |
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 understand your drive for simplicity, which I can see very well in this well factored project, and I think your way is better ascetically. I like the idea of removing the whitelists and blacklists completely.
Never the less this approach could lead to an API that's too raw, giving user the ability to shoot them self's in the foot.
- A user provided error handler will have to know about the special classes in the default handler, in some cases, in order not to rescue critical errors.
- There could be system side effect with having to re-raise the error, especially since there are such critical errors.
I think there are some very smart people working over at RSpec and would prefer their Module::===
method for validating what errors to rescue.
https://github.com/rspec/rspec-support/blob/master/lib/rspec/support.rb#L122
It may also make for a better Proc
API to return true
if the user want Stoplight to handle the issue and false
for Stoplight to ignore it. This would take a little meta programming to create a dynamic module that takes the user supplied handler.
Sorry for changing my mind so many times.
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 will create a PR, based off yours to show in code what I mean.
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.
Thanks for the feedback!
- I agree that this is not so nice. You can fall back to the default handler as the last line of your custom handler:
return Stoplight::Default::ERROR_HANDLER.call(error)
. That's a little wordy, so people probably won't do it. - I don't think there's any problem with catching and re-raising errors.
- The difference between my method and RSpec's method is cosmetic.
- I considered this approach, but ultimately decided against it because it could be confusing. Does
true
mean "re-raise the error", or is thatfalse
? By explicitly callingraise
in your handler, there is no confusion. Also, the only thing Stoplight would ever do to handle an error is re-raise it, so why have that extra layer of indirection?
I think the first point is the biggest problem with this approach. There may be an easy way to fix it, like returning some sentinel value to use the default handler.
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 do agree that the true
/false
meaning is not apparent. What about a passed in object that responds with #handle_error(error)
to be called in the case where the user want it to affect stoplight.
Once this has some tests it will be good to go. |
This will fix #91. It still needs some tests.
2a12612
to
0ce5cdd
Compare
This will fix #91. It still needs some tests.