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: Rule "eqeqeq" to have more specific null handling (fixes #6543) #6849
Conversation
@sstur, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @pedrottimark and @BYK to be potential reviewers |
Thanks for the pull request, @sstur! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
LGTM |
@sstur Looks like this will need to be rebased. Can you rebase the branch? Let us know if you need help with that. |
LGTM |
@platinumazure: Done. Thanks! |
@@ -67,6 +67,13 @@ foo === null | |||
|
|||
``` | |||
|
|||
This rule optionally takes a second argument which should be an object with the following supported properties: |
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.
Suggest adding a comma after "second argument"- feels like that might read better.
Left a few minor comments. Thanks for submitting the PR, can't wait to see it land. |
Those all seem very doable. I'll get an update out... |
LGTM |
LGTM |
@platinumazure: done. |
One comment about schema, otherwise LGTM. |
LGTM, I think we should go with the proposed schema change. |
LGTM |
Thanks @gyandeeps and @ilyavolodin! Done as proposed by @mysticatea. |
LGTM 👍 |
What issue does this pull request address?
This fixes issue #6543: Enforce use of non-strict equality when comparing to
null
What changes did you make? (Give an overview)
This changes the rule
eqeqeq
so that it optionally takes a second argument which should be an object with one supported property:{"null": "always|never|ignore"}
. This lets us configure very specific handling of null literals. This also deprecates the option "allow-null" in favor of this more explicit config.Is there anything you'd like reviewers to focus on?
Please let me know if my code style fits your project and give me any feedback about my style of describing this feature in the readme. If you prefer better wording/terminology, comment with specifically how you would like to have it worded and I'll update the PR.
Thanks!