Skip to content

Make Raven.wrap respect prototype chain #402

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

Merged
merged 1 commit into from
Oct 31, 2015

Conversation

jlfwong
Copy link
Contributor

@jlfwong jlfwong commented Oct 31, 2015

See #401

Disclaimer: I didn't clone the repo and test this, I'm just sorta hoping this works, and using this as clearer documentation of the problem in #401.

@mattrobenolt
Copy link
Contributor

@benvinegar This seems sane to me, but I'm not 100% sure.

Thanks @jlfwong. 🍰

@mattrobenolt
Copy link
Contributor

Interesting that this didn't come from the loop before it when copying all the properties over.

This seems reasonable to me then. I'll cut a new version of this for you. :)

mattrobenolt added a commit that referenced this pull request Oct 31, 2015
Make Raven.wrap respect prototype chain
@mattrobenolt mattrobenolt merged commit b778ae0 into getsentry:master Oct 31, 2015
@mattrobenolt
Copy link
Contributor

Hmm, hey @jlfwong, so I was going to cut a release, then I noticed that this broke a test. Somehow I didn't catch this before merging.

So this seems to conflict with #130 considering there was a test to explicitly check and make sure we didn't copy prototype. Can you shed some light here possibly?

@mattrobenolt
Copy link
Contributor

Also... not sure why Travis isn't running tests for Pull Requests. Then I'd have noticed this before merging. :/ Ugh.

@mattrobenolt
Copy link
Contributor

I'm inclined to think we can just remove that one test and it should be ok.

@jlfwong jlfwong deleted the patch-1 branch November 2, 2015 07:24
@jlfwong
Copy link
Contributor Author

jlfwong commented Nov 2, 2015

Thanks for the merge!

@mattrobenolt
Copy link
Contributor

@jlfwong Can you address my comments above please? :/

@jlfwong
Copy link
Contributor Author

jlfwong commented Nov 2, 2015

I don't understand why the test to ensure the prototype isn't copied exists at all. It seems reasonable to me to just remove that test

@mattrobenolt
Copy link
Contributor

Thanks. That's my assumption as well.

I'm just waiting on feedback from @benvinegar as a double check, and we'll be good here.

Thanks again. :)

@benvinegar
Copy link
Contributor

I don't understand why the test to ensure the prototype isn't copied exists at all. It seems reasonable to me to just remove that test

Agreed. I think this patch is correct (hi @jlfwong).

Hopefully this doesn't bite anyone randomly when upgrading.

mattrobenolt added a commit that referenced this pull request Nov 2, 2015
@jlfwong
Copy link
Contributor Author

jlfwong commented Nov 2, 2015

Thanks! (hi @benvinegar!)

Is a new release going to be cut?

@mattrobenolt
Copy link
Contributor

Yeah, I'm going to review everything and will get it cut today.

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