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

Update for compat with chainable method overwriting #87

Closed
wants to merge 1 commit into from

Conversation

joshperry
Copy link
Contributor

No description provided.

@domenic
Copy link
Collaborator

domenic commented Feb 25, 2015

Tests? What does this fix?

@joshperry
Copy link
Contributor Author

I didn't change or add functionality, I assumed the existing tests would suffice to ensure there was no change in that regard. I am happy to write some if you feel there is a need.

This fixes the code calling addChainableMethod to overwrite existing methods when chai has overwriteChainableMethod for that purpose. This makes it difficult to manage reentrancy in other plugins that also need to overwrite existing chainable methods.

Also, detecting chainable methods by calling the getters in a try/catch to see if they return a function is error prone if you don't know the preconditions that the getter expects. This instead looks at the chainable method registry that chai keeps.

@domenic
Copy link
Collaborator

domenic commented Feb 25, 2015

If there's no change or add to functionality, then there's no reason to merge this. It sounds like there are changes, from your description; there need to be tests for that then.

@domenic
Copy link
Collaborator

domenic commented Mar 5, 2015

Ping. Would love tests on this. I noticed from the referenced issue you seem to think I don't like you and am dead-set against this? I can assure you that's not the case.

Ryckes added a commit to Ryckes/chai-as-promised that referenced this pull request Mar 16, 2015
@domenic
Copy link
Collaborator

domenic commented Mar 24, 2015

Superceded by #94, which included tests.

@domenic domenic closed this Mar 24, 2015
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