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

Add overwriteChainableMethod utility #219

Merged
merged 3 commits into from Dec 20, 2013

Conversation

@maxedmands
Copy link

@maxedmands maxedmands commented Nov 30, 2013

Here you go. I ended up storing the original method and chaining behavior in a global object, which makes me feel a bit dirty, but there it is. Not sure how else this could be done without significantly modifying the plugin API.

Ref: #215.

@coveralls
Copy link

@coveralls coveralls commented Nov 30, 2013

Coverage Status

Coverage remained the same when pulling fe192c5 on demands:overwrite_chainable into 0e560c6 on chaijs:master.

@maxedmands
Copy link
Author

@maxedmands maxedmands commented Nov 30, 2013

Oops. Karma tests don't pass. Attempting to fix...

@maxedmands
Copy link
Author

@maxedmands maxedmands commented Nov 30, 2013

There. Forgot to add the new file to components.json.

@coveralls
Copy link

@coveralls coveralls commented Nov 30, 2013

Coverage Status

Coverage remained the same when pulling 2127525 on demands:overwrite_chainable into 0e560c6 on chaijs:master.

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Nov 30, 2013

Do you have a SauceLabs account so you can run the full browser test suite? If not you can get an Open Sauce account or I can provide temporary credentials:

// test/auth/index.js
module.exports = {
  SAUCE_USERNAME: "[username]",
  SAUCE_ACCESS_KEY: "[access key]"
};
$ make test-sauce
@maxedmands
Copy link
Author

@maxedmands maxedmands commented Nov 30, 2013

Right on, thanks :) Running them now.

@maxedmands
Copy link
Author

@maxedmands maxedmands commented Nov 30, 2013

Looks like they all passed. Never used saucelabs before! I am impressed :)

@logicalparadox

This comment has been minimized.

Since this can be applied to different contexts (though we only do it for Assertion.prototype currently) it would be better if we didn't memoize the methods globally. Perhaps it would be better on ctx.__methods?

This comment has been minimized.

Copy link
Owner

@maxedmands maxedmands replied Nov 30, 2013

I considered augmenting ctx to memoize the methods, but I was worried that modifying it might create some weird side effects.

That's why addChainableMethod.methods[name] is an array: each object in the array contains a reference to ctx, and there's a loop through all the objects in overwriteChainableMethod to look up the appropriate memoized method.

That's the black magic, I guess. If you think it's fine to add something like ctx.__methods, that would definitely make for cleaner / simpler code. Just so long as nobody ever wants to use __methods for something else.

@logicalparadox

This comment has been minimized.

code style: comma first please

This comment has been minimized.

Copy link
Owner

@maxedmands maxedmands replied Nov 30, 2013

Oops. Sorry!

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Nov 30, 2013

Comments inline ... let me know your thoughts.

@coveralls
Copy link

@coveralls coveralls commented Nov 30, 2013

Coverage Status

Coverage remained the same when pulling 576d890 on demands:overwrite_chainable into 0e560c6 on chaijs:master.

Max Edmands
@coveralls
Copy link

@coveralls coveralls commented Nov 30, 2013

Coverage Status

Coverage remained the same when pulling 76ac685 on demands:overwrite_chainable into 0e560c6 on chaijs:master.

@coveralls
Copy link

@coveralls coveralls commented Dec 1, 2013

Coverage Status

Coverage remained the same when pulling 270f9d9 on demands:overwrite_chainable into 0e560c6 on chaijs:master.

@maxedmands
Copy link
Author

@maxedmands maxedmands commented Dec 20, 2013

Ping -- what can I do to get this merged?

@vesln
Copy link
Member

@vesln vesln commented Dec 20, 2013

I'm happy to merge it in, however since @logicalparadox did the initial comments, it I'd prefer to wait for him to do so.

logicalparadox added a commit that referenced this pull request Dec 20, 2013
Add overwriteChainableMethod utility
@logicalparadox logicalparadox merged commit 564af34 into chaijs:master Dec 20, 2013
1 check passed
1 check passed
@logicalparadox
default The Travis CI build passed
Details
@maxedmands maxedmands deleted the maxedmands:overwrite_chainable branch Jan 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants