This repository was archived by the owner on Apr 8, 2020. It is now read-only.
Feature: handle rescue_from Module arguments properly#19
Merged
exAspArk merged 1 commit intoexAspArk:masterfrom Nov 12, 2019
Merged
Feature: handle rescue_from Module arguments properly#19exAspArk merged 1 commit intoexAspArk:masterfrom
exAspArk merged 1 commit intoexAspArk:masterfrom
Conversation
There is a common pattern for `rescue_from` and alike methods usage, to pass as an argument a module which handles similar exceptions processing. The most useful case (and the reason why I proposed that change) is to handle different network errors with only on class. In our project it's called Handle::NetworkErrors, it's a module which aggregates handling of more then 20 different errors whose meaning is just "something was wrong with the network" and in 99% of the cases it's reasonable to handle them in exact similar way. Unfortunately, without proposed change it's impossible to pass to graphql-errors `rescue_from` a module. It has to be a class... The other implementations of same method in Rails and Sidekiq accepts Module as well as a Class. So, my point is that would be great to have `rescue_from` behavior in a cannonical way (or you may call it, principle of least surprise).
Contributor
Author
|
@exAspArk mentioned you here, just in case 🙏 |
exAspArk
reviewed
Nov 12, 2019
| describe '#rescue_from' do | ||
| it 'raises an exception if the object which was passed is not a class' do | ||
| before do | ||
| module NetworkErrors |
Owner
There was a problem hiding this comment.
we could potentially define it outside the before block which is being triggered for every test to avoid:
warning: already initialized constant NetworkErrors::ERRORS
Owner
|
@sclinede amazing! Released your changes in version Спасибо за пул реквест и развернутое описание :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There is a common pattern for
rescue_fromand alike methods usage, topass as an argument a module which handles similar exceptions
processing. The most useful case (and the reason why I proposed that
change) is to handle different network errors with only on module.
In our project it's called Handle::NetworkErrors, it's a module which
aggregates handling of more then 20 different errors whose meaning is
just "something was wrong with the network" and in 99% of the cases it's
reasonable to handle them in exact similar way.
Unfortunately, without proposed change it's impossible to pass to
graphql-errors
rescue_froma module. It has to be a class...The other implementations of same method in Rails and Sidekiq accepts
Module as well as a Class.
So, my point is that would be great to have
rescue_frombehavior in acanonical way (or you may call it, principle of least surprise).
Hope you'll find it useful and thanks for your nice gem.