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

Better error handling in Chewy::Strategy#wrap #306

Merged

Conversation

barthez
Copy link
Contributor

@barthez barthez commented Jan 5, 2016

Passing non-exising strategy to Chewy::Strateg#wrap caused unclear error: "Can't pop root strategy". Now, push and wrap give back better error when strategy is missing.

Passing non-exising strategy to Chewy::Strategy#wrap caused
unclear error: "Can't pop root strategy". Now, push and wrap
give back better error when strategy is missing.
@barthez barthez force-pushed the better-error-handling-in-strategy-wrap branch from 6ad7b9c to dea0a5d Compare January 6, 2016 10:34
@barthez
Copy link
Contributor Author

barthez commented Jan 6, 2016

It turns out that with mongoid we can observe strange behavior of safe_constantize (const_get in particular). It turns you that Foo.const_get('Bar') raises error with message uninitialized constant Bar (what cannot be handled by safe_constantize from ActiveSupport < 4.2), but Foo.const_get(:Bar) raises error with message uninitialized constant Foo::Bar and further calls of const_get with string will raise "proper" error message, with full qualified constant.

I've introduced small workaround which handle that case.

pyromaniac added a commit that referenced this pull request Jan 6, 2016
…y-wrap

Better error handling in Chewy::Strategy#wrap
@pyromaniac pyromaniac merged commit 721b2db into toptal:master Jan 6, 2016
@pyromaniac
Copy link
Contributor

Perfect, thanks!

@barthez barthez deleted the better-error-handling-in-strategy-wrap branch January 20, 2016 10:16
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.

2 participants