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

fix: do not suppress errors when loading classes #309

Merged

Conversation

andreynering
Copy link
Contributor

This rescue block was suppressing real errors on our codebase. I had to remove this block on a local fork to be able to see the real cause of a bug that was silently happening.

I noticed that removing the blocks do not break any tests. Looks like we should just remove them?

@kbrock
Copy link
Collaborator

kbrock commented May 6, 2024

@andreynering Thanks. good catch. I think it was missed when we converted the constantize to safe_constantize in #297

Can you share the example of what error was being squashed here? (for my own personal knowledge - no code necessary but a quick description would be appreciated)

I did have concerns with the constantize here since loading the class here has caused race conditions in another codebase. But the added benefits of checking this class outweighed the rare issue.

Do you think you could put together a test here for when we specify an invalid class? (if it is super hard, then just say so and we can punt.


@flavorjones I missed this try/catch when you added the safe_constantize.
I'm assuming that we just missed it and I'd like to merge this change.

But I do have to ask: Did you leave this try/catch for a particular reason?
There is an issue when the inflection for the class is slightly different. (e.g.: class named VM but we try and load Vm) Is this the reason you left it in? Or was it just an oversight? This is a very rare edge case, but wanted to double check.

@andreynering
Copy link
Contributor Author

@kbrock We had a concern included on a given ActiveRecord model, but the content of included do; end block wasn't actually been ran. Ends up there was an error being raised, but that block was suppressing it.

Copy link
Collaborator

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with it!

@kbrock kbrock merged commit 24579e9 into active-hash:master May 6, 2024
16 checks passed
@andreynering andreynering deleted the chore/remove-exception-supression branch May 6, 2024 17:46
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