tools: auto fix custom eslint rule for prefer-assert-methods.js #16652

shobhitchittora · 2017-10-31T20:53:54Z

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); to assert.notStrictEqual(obj.value, 9);

Refs: #16636

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines
modify tests

Affected core subsystem(s)

Tools

shobhitchittora · 2017-11-02T14:55:36Z

Hi @apapirovski. Can you also review this one? Thanks in advance.

BridgeAR

In general LGTM but it should be extended to assert.ok() as assert() is actually just the alias.

addaleax · 2017-11-30T02:12:46Z

CI: https://ci.nodejs.org/job/node-test-commit/14472/

apapirovski · 2017-12-09T20:53:59Z

ping @shobhitchittora — would you like to follow up on this? There's a bit of feedback here from @BridgeAR.

shobhitchittora · 2017-12-10T06:08:39Z

@apapirovski @BridgeAR extended for assert.ok. Please review.

BridgeAR · 2017-12-10T06:12:50Z

test/parallel/test-eslint-prefer-assert-methods.js

+        message: "'assert.ok' should be used instead of alias 'assert'"
+      }],
+      output: 'assert.ok(val);'
+    }


I would not prefer assert.ok over assert. Both should be fine out of my perspective.

I'm a bit confused here. What did you mean when you said extend for assert.ok()?

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().

Thanks for clarifying this. I'll revert the added invalid test for assert(val).

apapirovski · 2017-12-10T20:53:44Z

test/parallel/test-eslint-prefer-assert-methods.js

+    'assert.notEqual(foo, bar);',
+    'assert(foo != bar && baz);',
+    'assert.ok(foo);',
+    'assert.ok(foo != bar);',


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

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

shobhitchittora · 2017-12-15T10:28:07Z

@BridgeAR @apapirovski Updated the PR as per the new implementation by @cjihrig.

BridgeAR · 2018-01-19T17:51:40Z

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 · 2018-02-01T10:41:24Z

Landed in 2d6912a

1. Extends tests 2. Refactors code 3. Adds fixer Refs: #16636 PR-URL: #16652 Refs: #16636 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 31, 2017

BridgeAR approved these changes Nov 23, 2017

View reviewed changes

addaleax approved these changes Nov 30, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

apapirovski added the wip Issues and PRs that are still a work in progress. label Dec 9, 2017

BridgeAR reviewed Dec 10, 2017

View reviewed changes

shobhitchittora force-pushed the pref-assert-methods-eslint-fixer branch from 246c566 to 74b89ca Compare December 10, 2017 06:41

apapirovski reviewed Dec 10, 2017

View reviewed changes

tools: auto fix custom eslint rule for prefer-assert-methods.js

16e64b5

1. Extends tests 2. Refactors code 3. Adds fixer Refs: nodejs#16636

shobhitchittora force-pushed the pref-assert-methods-eslint-fixer branch from 74b89ca to 16e64b5 Compare December 15, 2017 08:11

updated as per new implementation

0ee62da

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jan 31, 2018

BridgeAR closed this Feb 1, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added dont-land-on-v6.x labels Mar 20, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: auto fix custom eslint rule for prefer-assert-methods.js #16652

tools: auto fix custom eslint rule for prefer-assert-methods.js #16652

shobhitchittora commented Oct 31, 2017 •

edited

Loading

shobhitchittora commented Nov 2, 2017

BridgeAR left a comment

addaleax commented Nov 30, 2017

apapirovski commented Dec 9, 2017

shobhitchittora commented Dec 10, 2017

BridgeAR Dec 10, 2017

shobhitchittora Dec 10, 2017 •

edited

Loading

BridgeAR Dec 10, 2017

shobhitchittora Dec 10, 2017

shobhitchittora Dec 10, 2017

apapirovski Dec 10, 2017 •

edited

Loading

BridgeAR Dec 10, 2017

shobhitchittora commented Dec 15, 2017

BridgeAR commented Jan 19, 2018

BridgeAR commented Feb 1, 2018

tools: auto fix custom eslint rule for prefer-assert-methods.js #16652

tools: auto fix custom eslint rule for prefer-assert-methods.js #16652

Conversation

shobhitchittora commented Oct 31, 2017 • edited Loading

Checklist

Affected core subsystem(s)

shobhitchittora commented Nov 2, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

addaleax commented Nov 30, 2017

apapirovski commented Dec 9, 2017

shobhitchittora commented Dec 10, 2017

BridgeAR Dec 10, 2017

Choose a reason for hiding this comment

shobhitchittora Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR Dec 10, 2017

Choose a reason for hiding this comment

shobhitchittora Dec 10, 2017

Choose a reason for hiding this comment

shobhitchittora Dec 10, 2017

Choose a reason for hiding this comment

apapirovski Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR Dec 10, 2017

Choose a reason for hiding this comment

shobhitchittora commented Dec 15, 2017

BridgeAR commented Jan 19, 2018

BridgeAR commented Feb 1, 2018

shobhitchittora commented Oct 31, 2017 •

edited

Loading

shobhitchittora Dec 10, 2017 •

edited

Loading

apapirovski Dec 10, 2017 •

edited

Loading