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: Handle Schrodingers Error #1261

Merged
merged 2 commits into from
Mar 15, 2018
Merged

fix: Handle Schrodingers Error #1261

merged 2 commits into from
Mar 15, 2018

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek requested a review from MaxBittker March 15, 2018 15:22
Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

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

code change looks good to me, nice fix 👍

only nit is a clearer explanation of what causes this or what these errors look like, but it's not blocked if this fix need to go out

@@ -3079,6 +3081,29 @@ describe('Raven (public API)', function() {
});
}

it("should treat Schrodinger's Error in the same way as regular Error", function() {
// Schrodinger's Error is an object that is and is not an Error at the same time
Copy link
Contributor

Choose a reason for hiding this comment

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

could you characterize the properties of these errors a little bit more explicitly (like, "it has the prototype of an error but also passes isPlanObject because X", or maybe link the the issue? i'm not sure i would understand based only on this test

@zivl
Copy link
Contributor

zivl commented Mar 15, 2018

@kamilogorek looks good to me as well!
much kudos for the fast response

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