Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
tools: auto fix custom eslint rule for prefer-assert-methods.js#16652
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
tools: auto fix custom eslint rule for prefer-assert-methods.js #16652
Uh oh!
There was an error while loading. Please reload this page.
Conversation
shobhitchittora commented Oct 31, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
shobhitchittora commented Nov 2, 2017
Hi @apapirovski. Can you also review this one? Thanks in advance. |
BridgeAR left a comment
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.
In general LGTM but it should be extended to assert.ok() as assert() is actually just the alias.
addaleax commented Nov 30, 2017
apapirovski commented Dec 9, 2017
ping @shobhitchittora — would you like to follow up on this? There's a bit of feedback here from @BridgeAR. |
shobhitchittora commented Dec 10, 2017
@apapirovski@BridgeAR extended for |
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 would not prefer assert.ok over assert. Both should be fine out of my perspective.
shobhitchittoraDec 10, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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'm a bit confused here. What did you mean when you said extend for assert.ok()?
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.
In general assert() should be treated identical to assert.ok(). And I meant the tests should be extended to test for both. Before I commented there were only tests for assert().
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.
Thanks for clarifying this. I'll revert the added invalid test for assert(val).
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.
246c566 to 74b89caCompare
apapirovskiDec 10, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 could be wrong but I think the idea was that assert(foo != bar) should yield the same as assert.ok(foo != bar). Since the former is an alias for the latter. That is, they should both report an error.
(I realize that might be modifying the current rule and is somewhat outside of the scope of the original work.)
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.
That would indeed be nice but I guess it is best to keep that for a separate PR and I am actually about to improve the assert message for cases like that in #17581
1. Extends tests 2. Refactors code 3. Adds fixer Refs: nodejs#16636
74b89ca to 16e64b5Compareshobhitchittora commented Dec 15, 2017
@BridgeAR@apapirovski Updated the PR as per the new implementation by @cjihrig. |
BridgeAR commented Jan 19, 2018
Mini-CI (enough for this test): https://ci.nodejs.org/job/node-test-commit-light/149/ |
1. Extends tests 2. Refactors code 3. Adds fixer Refs: nodejs#16636 PR-URL: nodejs#16652 Refs: nodejs#16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR commented Feb 1, 2018
Landed in 2d6912a |
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1. Extends tests 2. Refactors code 3. Adds fixer Refs: nodejs#16636 PR-URL: nodejs#16652 Refs: nodejs#16636 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
This adds eslint fixer for auto-fixing the usage of assert operators. Also adding fileoverview for the perfer-assert-methods.js file.
For example the fixer change this
assert(obj.value !== 9);toassert.notStrictEqual(obj.value, 9);Refs: #16636
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
Tools