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

Don't shadow gem loading errors during autoloading with the inline adapter #1486

Merged

Conversation

Earlopain
Copy link
Collaborator

@Earlopain Earlopain commented Sep 12, 2024

Gem::LoadError does not inherit from StandardError and as such won't be rescued. Just let it bubble up.

That would already happen but since a different error occurs in ensure, it was shadowed. I briefly debated rescuing other types of errors here but quickly abandoned that idea.

Closes #1485


There's a bit of churn in todo.rbi, I regenerated it with bin/tapioca todo (apparently that command is deprecated 🤷)

`Gem::LoadError` does not inherit from `StandardError` and as such won't be rescued.
Just let it bubble up.

Closes bensheldon#1485
@Earlopain Earlopain force-pushed the perform_later-autoloading-error branch from 9ef82bb to 70d729b Compare September 12, 2024 14:28
@bensheldon
Copy link
Owner

I think this change is good.

I have been slowly introducing more ensure usage into my reportoire (rather than rescue and re-raise), so this is useful to see where it fails.

Aside: seeing the re-raise with fresh eyes, I imagine that might mess up the previous stacktrace.

There's a bit of churn in todo.rbi

ugh. I've been manually adding/managing that file (probably should rename it already or note that).

@bensheldon bensheldon added the bug Something isn't working label Sep 12, 2024
@bensheldon bensheldon merged commit 5152a7b into bensheldon:main Sep 12, 2024
10 checks passed
@Earlopain
Copy link
Collaborator Author

I've been manually adding/managing that file (probably should rename it already or note that).

I ended up adding it manually as well. Generating it automatically missed some cases, probably because it doesn't see things in stub_const.

seeing the re-raise with fresh eyes, I imagine that might mess up the previous stacktrace.

It may be fine? At least in tests the exception seems to come from the correct place.

@Earlopain Earlopain deleted the perform_later-autoloading-error branch September 12, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

Undefined method unhandled_error for nil
2 participants