-
-
Notifications
You must be signed in to change notification settings - Fork 699
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
Make proxies optional #770
Conversation
/** | ||
* ### config.useProxy | ||
* | ||
* User configurable property, define if chai will use a Proxy to throw |
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.
Nitpicky Warning: The word define
here should be written as defines
, shouldn't it?
(Please correct me if I'm wrong, I'm not a native speaker)
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.
Agreed on "defines" over "define".
Also maybe be a bit more explicit on the purpose of this feature. For example adding onto the end:
"...non-existent property is read, which protects users from typos when using property-based assertions."
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.
Agreed with both you guys. I will add the suggestions.
Thanks
BTW I forgot to thank you for this awesome PR. |
5330fcc
to
400180a
Compare
@vieiralucas Awesome work sir. Thanks for doing this :D It looks like you already fixed one of the two line comments I made while reviewing. Once the other is resolved I think this is set to merge. Btw, how does one go about joining the Lucas Syndicate? Is being named Lucas a hard requirement? |
400180a
to
9cfb3b8
Compare
LGTM :D |
@meeber Thank you, and all the maintainers of chai for making this project such a friendly place to new contributors. I believe that being named Lucas would be a requirement for joining our Syndicate, but maybe we can add an exception for you. |
* an error when a non-existent property is read, which protects users | ||
* from typos when using property-based assertions. | ||
* | ||
* Set it to false if you want to disable this feature. |
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.
I just had another thought on this: We should mention that this feature is automatically disabled regardless of this config value in environments that don't support proxies.
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.
Agreed, should I put this before the example or after?
/**
* ### config.useProxy
*
* User configurable property, defines if chai will use a Proxy to throw
* an error when a non-existent property is read, which protects users
* from typos when using property-based assertions.
* This feature is automatically disabled regardless of this config value
* in environments that don`t support proxies.
*
* Set it to false if you want to disable this feature.
*
* chai.config.useProxy = false; // disable use of Proxy
*
* @param {Boolean}
* @api public
*/
vs
/**
* ### config.useProxy
*
* User configurable property, defines if chai will use a Proxy to throw
* an error when a non-existent property is read, which protects users
* from typos when using property-based assertions.
*
* Set it to false if you want to disable this feature.
*
* chai.config.useProxy = false; // disable use of Proxy
*
* This feature is automatically disabled regardless of this config value
* in environments that don`t support proxies.
*
* @param {Boolean}
* @api public
*/
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.
I like the second
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.
Done
9cfb3b8
to
0f34cb5
Compare
LGTM |
LGTM too. |
Hello, this is my proposal for solving #766
First of all, thank you @meeber for describing what, where and how to do this.
All I had to do was add the code. You are awesome.
Please let me know if I'm missing something here.
Thank you all ❤️