-
-
Notifications
You must be signed in to change notification settings - Fork 697
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
Wrap non-chainable methods in proxies #789
Conversation
if (nonChainableMethodName) { | ||
throw Error('Invalid use of Chai method: ' + nonChainableMethodName + | ||
'. Invocation required.'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message is a bit unfriendly, I feel like it doesn't explicitly point out where the user went wrong. Could we instead have something like Invalid Chai property equal.pizza. Did you mean equal(pizza)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keithamus Agreed it can be better. I struggled with the phrasing quite a bit actually. The reason I ended up with the current phrasing is because many of the alternatives I explored only offer sensible advice part of the time.
For example:
expect(x).to.be.equal.to.true;
// Invalid Chai property: equal.to. Did you mean equal(to)?
expect(x).to.be.a.string.with.length(4);
// Invalid Chai property: string.with. Did you mean string(with)?
expect(x).to.throw.an.error;
// Invalid Chai property: throw.an. Did you mean throw(an)?
expect(x).to.be.within.2.and.4;
// Invalid Chai property: within.2. Did you mean within(2)?
I feel uncomfortable with Chai reacting to the user's mistake by suggesting an alternative that's also incorrect, especially in the last case since it demonstrates using within
with only one argument when it actually requires two.
Could do something like: Invalid Chai property: equal.pizza. See docs for proper usage of the "equal" assertion
.
Or the high-effort solution with customized messages for each method: Invalid usage of .within. Correct usage: .within(start, finish)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I like the idea of "See docs...". I agree that we dont want to present wrong info, but I also what to give users a chance to fix without immediately resorting to a support channel (like our issue tracker 😵)
6d2c663
to
e28f048
Compare
e28f048
to
eac90a5
Compare
This one is ready for review. But first, a special shout out to:
|
Okay, this LGTM. Great work @meeber 👍 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, LGTM
ps: I like pizza to 🍕
LGTM github review system pls |
Extends proxy protection to non-chainable methods (including overwritten non-chainable methods) with a helpful error message. Example: