Uh oh!
There was an error while loading. Please reload this page.
tools: auto fix custom eslint rule for prefer-assert-methods.js - #16652
tools: auto fix custom eslint rule for prefer-assert-methods.js#16652shobhitchittora wants to merge 2 commits into
Conversation
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.
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.
I would not prefer assert.ok over assert. Both should be fine out of my perspective.
There was a problem hiding this comment.
I'm a bit confused here. What did you mean when you said extend for assert.ok()?
There was a problem hiding this comment.
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.
Thanks for clarifying this. I'll revert the added invalid test for assert(val).
246c566 to
74b89caCompareThere was a problem hiding this comment.
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.
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 <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR
commented
Feb 1, 2018
Landed in 2d6912a |
1. Extends tests 2. Refactors code 3. Adds fixer Refs: nodejs#16636 PR-URL: nodejs#16652 Refs: nodejs#16636 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>
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