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

Prevent FallbackScan from polluting exception cause #314

Merged
merged 1 commit into from Aug 5, 2020

Conversation

aaronjensen
Copy link
Contributor

When debugging an issue with test-bench and bootsnap, I discovered that bootsnap pollutes error.cause by requiring the file naturally after a fallback scan within a rescue. This causes any error that is thrown within the required file to have a #cause defined, which it should not. This improves output in test-bench when an error occurs and it may (I haven't tested this) improve output in other frameworks that inspect error.cause.

I wasn't sure how to write a test for this because I didn't know how to trigger a fallback scan (I don't even really know what that is) and still load a real file that raises an exception. If you could give me a snippet for that I can add a test.

Thanks!

@aaronjensen aaronjensen requested a review from burke as a code owner July 31, 2020 03:57
@ghost ghost added the cla-needed label Jul 31, 2020
@sbellware
Copy link

The maintainer might just take this code as a proof-of-concept and make the changes themselves. That would obviate the CLA hoop-jumping and delays.

@ghost ghost removed the cla-needed label Jul 31, 2020
@burke
Copy link
Member

burke commented Aug 5, 2020

LGTM. We'll live without a test for this.

@burke burke merged commit b1096d1 into Shopify:master Aug 5, 2020
@aaronjensen aaronjensen deleted the fix-error-cause-pollution branch August 5, 2020 16:18
@sbellware
Copy link

Awesome ❤️

@aaronjensen
Copy link
Contributor Author

Thank you for the merge!

If you have a free moment to cut a new gem release, I'd really appreciate it. Thank you.

@burke
Copy link
Member

burke commented Aug 11, 2020

Done!

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.

None yet

4 participants