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

Make proxies optional #770

Merged
merged 1 commit into from Aug 15, 2016
Merged

Conversation

vieiralucas
Copy link
Member

@vieiralucas vieiralucas commented Aug 13, 2016

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 ❤️

@vieiralucas vieiralucas changed the title Make proxies optional Fix #766 Make proxies optional Aug 13, 2016
/**
* ### config.useProxy
*
* User configurable property, define if chai will use a Proxy to throw
Copy link
Member

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)

Copy link
Contributor

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."

Copy link
Member Author

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

@lucasfcosta
Copy link
Member

BTW I forgot to thank you for this awesome PR.
Since we get to talk to each other almost everyday on college I forgot that.
Just left some minor notes about grammar and another thought I had about test's coverage.

@vieiralucas vieiralucas force-pushed the optional-proxy branch 2 times, most recently from 5330fcc to 400180a Compare August 14, 2016 20:29
@meeber
Copy link
Contributor

meeber commented Aug 14, 2016

@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?

@meeber
Copy link
Contributor

meeber commented Aug 14, 2016

LGTM :D

@vieiralucas
Copy link
Member Author

@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.
Copy link
Contributor

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.

Copy link
Member Author

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
   */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the second

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@meeber
Copy link
Contributor

meeber commented Aug 15, 2016

LGTM

@lucasfcosta
Copy link
Member

lucasfcosta commented Aug 15, 2016

LGTM too.

@lucasfcosta lucasfcosta merged commit 0662b7b into chaijs:master Aug 15, 2016
@meeber meeber mentioned this pull request Aug 15, 2016
@vieiralucas vieiralucas deleted the optional-proxy branch August 15, 2016 19:00
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.

None yet

3 participants