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

Added failing spec for nested rescue_from declarations #1982

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wowinter13
Copy link

See: #1975

@grape-bot
Copy link

grape-bot commented Jan 21, 2020

1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Generated by 🚫 danger

@dblock
Copy link
Member

dblock commented Jan 21, 2020

Doesn't look like it's failing?

@wowinter13 wowinter13 force-pushed the fix/rescue_from_specs branch from 1880e88 to c55a9c7 Compare January 21, 2020 17:26
@wowinter13
Copy link
Author

wowinter13 commented Jan 21, 2020

Sorry, a bit of a misunderstanding
UPD: fixed

@wowinter13 wowinter13 requested a review from dblock January 21, 2020 19:06
@dblock
Copy link
Member

dblock commented Jan 21, 2020

Cool, now that we have a spec all we need is a fix! Give it a shot.

@wowinter13
Copy link
Author

@dblock, I have some ideas on how to fix it, but want to discuss due to lack of knowledge.

Somehow we need to use rescue_handler_for_any_class before any other handler, BUT only in case of nested rescue blocks. For now I don't see an obvious solution without stacking all errors (like :rescue_all, :rescue_grape_exceptions etc) to some namespace. Maybe there is any better realization like using reverse_stacking from descendants to the ancestor? Or..?

@dblock
Copy link
Member

dblock commented Jan 22, 2020

Feels like maybe the tree of rescue handlers should be traversed as a tree?

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

Successfully merging this pull request may close these issues.

3 participants