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 #867: Walk properties only once #868

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Conversation

mantoni
Copy link
Member

@mantoni mantoni commented Oct 6, 2015

This fixes createStubInstance for prototype chains with overrides as reported in #867. One could argue that the fix belongs into the iterator function in sinon.stub. I tried adding a check for whether a property is already stubbed and that also does the trick, but it seemed a bit messy.

Thoughts?

@fatso83
Copy link
Contributor

fatso83 commented Oct 6, 2015

Good work, I think this looks pretty clean. Just to see if I understand you correctly; the alternative would be to create an analog of your seen object in this part of sinon.stub, right? In that case, your take indeed seems cleaner

@mantoni
Copy link
Member Author

mantoni commented Oct 6, 2015

Alternatively, there could be an if-statement around sinon.stub(object, prop) to check whether it's already stubbed.

@mroderick
Copy link
Member

Needs a rebase

@mantoni mantoni force-pushed the walk-properties-only-once branch from 7e3e0b1 to 43ba5b4 Compare October 6, 2015 20:34
@mantoni
Copy link
Member Author

mantoni commented Oct 6, 2015

@mroderick was already on it ;)

mroderick added a commit that referenced this pull request Oct 6, 2015
@mroderick mroderick merged commit a63d821 into master Oct 6, 2015
@mroderick mroderick deleted the walk-properties-only-once branch October 6, 2015 20:43
@mroderick
Copy link
Member

Will you also make a PR into https://github.com/sinonjs/sinon/tree/v1.17 branch, so we can publish a new 1.17.x version?

@mroderick
Copy link
Member

(yes, we really need to get back to having master as the branch we're shipping from)

@mantoni
Copy link
Member Author

mantoni commented Oct 6, 2015

Ah, good point. Will do.

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