tools: auto fix custom eslint rule for prefer-assert-iferror.js #16648

shobhitchittora · 2017-10-31T20:02:24Z

This implements an eslint fixer function to replace if(err) throw err;, usages to assert.ifError(err);.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
Extended the current tests for custom eslint rule.
commit message follows commit guidelines
python ./tools/test.py parallel/test-eslint-prefer-assert-iferror

Affected core subsystem(s)

Tools

apapirovski

Thanks for starting the work on this @shobhitchittora — for all of these fixers, could you expand the existing tests located in test/parallel/test-eslint-*?

See https://eslint.org/docs/developer-guide/nodejs-api#ruletester and https://github.com/eslint/eslint/blob/master/tests/lib/rules/arrow-body-style.js for more info.

shobhitchittora · 2017-10-31T22:30:00Z

Thanks for pointing tests out. I'll start working on them after reading the docs you mentioned. Thanks again.

shobhitchittora · 2017-11-01T10:04:59Z

After going through test-eslint-prefer-assert-iferror.js, I didn't find any cases to add to the existing test rules. Please highlight if any.

apapirovski · 2017-11-01T11:18:45Z

The idea would be to expand the following:

    {
      code: 'if (err) throw err;',
      errors: [{ message: 'Use assert.ifError(err) instead.' }]
    },
    {
      code: 'if (error) { throw error; }',
      errors: [{ message: 'Use assert.ifError(error) instead.' }]
    }

with output which would test your fixer.

https://eslint.org/docs/developer-guide/nodejs-api#ruletester

output (string, optional): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the --fix command line flag). If this is null, asserts that none of the reported problems suggest autofixes.

shobhitchittora · 2017-11-01T18:57:55Z

Done. Extended the current tests with the output key. Expecting your review @apapirovski. Thanks.

apapirovski · 2017-11-01T19:21:29Z

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

    },
    {
      code: 'if (error) { throw error; }',
-      errors: [{ message: 'Use assert.ifError(error) instead.' }]
+      errors: [{ message: 'Use assert.ifError(error) instead.' }],
+      output: 'assert.ifError(error)'


These should be assert.ifError(err); and assert.ifError(error); with an ending semicolon.

@apapirovski Done. 👍🏻

apapirovski · 2017-11-01T19:21:29Z

tools/eslint-rules/prefer-assert-iferror.js

+            fix: (fixer) => {
+              return fixer.replaceText(
+                node,
+                `assert.ifError(${argument})`


You'll need a semicolon at the end.

@apapirovski Done. 👍🏻

apapirovski

LGTM — thanks for the adjustments @shobhitchittora

gibfahn

Seems correct (although I haven't tested it)

apapirovski · 2017-11-01T21:57:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11134/

shobhitchittora · 2017-11-02T13:05:41Z

Thank you all for being good sport. @apapirovski just wondering why the build is failing for linux and windows.

Finished Build : #13726 of Job : node-test-commit-linux with status : FAILURE
Finished Build : #13071 of Job : node-test-commit-windows-fanned with status : FAILURE

apapirovski · 2017-11-02T16:02:34Z

We have a test that's failing on those platforms. It's not related to this PR. :)

not-an-aardvark · 2017-11-04T03:02:14Z

This seems like it would cause incorrect code if the assert module has not been imported. This isn't necessarily a big problem (since assert would get reported as an undefined variable after the autofix), but it's possible that a user could get confused about it.

joyeecheung · 2017-11-06T14:44:00Z

@not-an-aardvark I think this comes down to what the expectation the user sets for the fixers...personally I would take it with a grain of salt and fix the tricky ones by myself before I run it to fix something like whitespace changes, but it may be just me.

BridgeAR · 2017-11-23T22:09:26Z

Hm, we could further check if assert is imported or not and fix that as well. But it might be a bit to much?

@not-an-aardvark what is your call about the PR?

apapirovski · 2017-11-23T22:10:47Z

Not a fan of fixing missing requires. It's pretty hard to detect where exactly the require needs to go within the file.

not-an-aardvark · 2017-11-23T22:16:09Z

I agree that it could be difficult to automatically insert require calls. I have no strong opinion either way on whether this would be worthwhile without inserting require calls.

Another option would be to only do an autofix if there is an assert variable in scope.

BridgeAR · 2017-11-23T22:17:29Z

Another option would be to only do an autofix if there is an assert variable in scope.

I think that would be best!

BridgeAR · 2017-12-06T04:25:50Z

@shobhitchittora would you be so kind and update the PR to also check if there is an assert variable in the scope and only fix it if that is the case?

shobhitchittora · 2017-12-10T05:28:49Z

@BridgeAR Done 👍🏻.

apapirovski · 2017-12-10T20:49:57Z

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

@@ -15,11 +15,13 @@ new RuleTester().run('prefer-assert-iferror', rule, {
  invalid: [
    {
      code: 'if (err) throw err;',
-      errors: [{ message: 'Use assert.ifError(err) instead.' }]
+      errors: [{ message: 'Use assert.ifError(err) instead.' }],
+      output: 'assert.ifError(err);'


I'm guessing this won't work without having require('assert'); in code and output. Same thing below.

@shobhitchittora would you be so kind and have a look at this comment?

The test would actually work without the require('assert') specified. While using assert the user has to require assert to use it right. Also in every other test for rules, this is what is followed.

As far as I can tell @apapirovski is right about his comment. I am going to run a CI anyway, so we do not have to speculate on it.

1. Adds the fixer method 2. Extends Tests Refs: nodejs#16636

BridgeAR · 2018-01-19T16:59:55Z

Mini CI (should be enough to test the rule): https://ci.nodejs.org/job/node-test-commit-light/145/

BridgeAR · 2018-01-19T17:08:07Z

not ok 410 parallel/test-eslint-prefer-assert-iferror
  ---
  duration_ms: 0.407
  severity: fail
  stack: |-
    /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:148
            throw err;
            ^
    
    AssertionError [ERR_ASSERTION]: Output is incorrect. ('if (err) throw err;' == 'assert.ifError(err);')

BridgeAR · 2018-01-31T02:05:03Z

Ping @shobhitchittora

shobhitchittora · 2018-01-31T02:44:15Z

@BridgeAR apologies for keeping this hanging. Been a little pre-occupied. Looking into it.😇

1. Fixes the failing test for the rule. Adds require("assert") to code and output. Refs: nodejs#16636

…ora/node into prefer-assert-iferror-fixer

shobhitchittora · 2018-02-04T04:21:19Z

@BridgeAR I pushed a change and now the tests are working locally. Used this to test - python ./tools/test.py parallel/test-eslint-prefer-assert-iferror.

Thanks for being so patient.

BridgeAR · 2018-02-07T03:14:43Z

CI https://ci.nodejs.org/job/node-test-pull-request/12988/

BridgeAR

LG if CI is green.

PR-URL: nodejs#16648 Refs: nodejs#16636 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2018-02-07T13:56:51Z

Landed in d797775 🎉

PR-URL: #16648 Refs: #16636 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#16648 Refs: nodejs#16636 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the tools label Oct 31, 2017

apapirovski reviewed Oct 31, 2017

View changes

apapirovski reviewed Nov 1, 2017

View changes

apapirovski approved these changes Nov 1, 2017

View changes

gibfahn approved these changes Nov 1, 2017

View changes

jasnell approved these changes Nov 2, 2017

View changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare Dec 8, 2017

apapirovski added the wip label Dec 9, 2017

apapirovski approved these changes Dec 10, 2017

View changes

apapirovski reviewed Dec 10, 2017

View changes

BridgeAR removed the wip label Dec 10, 2017

BridgeAR approved these changes Dec 10, 2017

View changes

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

94b97b0

1. Adds the fixer method 2. Extends Tests Refs: nodejs#16636

shobhitchittora force-pushed the prefer-assert-iferror-fixer branch from e62d6da to 94b97b0 Compare Dec 11, 2017

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

e061cba

1. Adds the fixer method 2. Extends Tests Refs: nodejs#16636

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare Jan 26, 2018

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare Jan 26, 2018

shobhitchittora added 2 commits Feb 4, 2018

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

756399c

1. Fixes the failing test for the rule. Adds require("assert") to code and output. Refs: nodejs#16636

Merge branch 'prefer-assert-iferror-fixer' of github.com:shobhitchitt…

d11af09

…ora/node into prefer-assert-iferror-fixer

BridgeAR added the author ready label Feb 7, 2018

BridgeAR approved these changes Feb 7, 2018

View changes

BridgeAR closed this Feb 7, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

gibfahn added dont-land-on-v6.x labels Apr 13, 2018

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

nodejs / node Public

tools: auto fix custom eslint rule for prefer-assert-iferror.js #16648

tools: auto fix custom eslint rule for prefer-assert-iferror.js #16648

shobhitchittora commented Oct 31, 2017 •

edited

apapirovski left a comment •

edited

shobhitchittora commented Oct 31, 2017

shobhitchittora commented Nov 1, 2017

apapirovski commented Nov 1, 2017 •

edited

shobhitchittora commented Nov 1, 2017 •

edited

apapirovski Nov 1, 2017 •

edited

shobhitchittora Nov 1, 2017

apapirovski Nov 1, 2017

shobhitchittora Nov 1, 2017

apapirovski left a comment

gibfahn left a comment

apapirovski commented Nov 1, 2017

shobhitchittora commented Nov 2, 2017

apapirovski commented Nov 2, 2017

not-an-aardvark commented Nov 4, 2017

joyeecheung commented Nov 6, 2017

BridgeAR commented Nov 23, 2017

apapirovski commented Nov 23, 2017

not-an-aardvark commented Nov 23, 2017

BridgeAR commented Nov 23, 2017

BridgeAR commented Dec 6, 2017

shobhitchittora commented Dec 10, 2017

apapirovski Dec 10, 2017 •

edited

BridgeAR Jan 5, 2018

shobhitchittora Jan 5, 2018

BridgeAR Jan 19, 2018

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 31, 2018 •

edited

shobhitchittora commented Jan 31, 2018

shobhitchittora commented Feb 4, 2018

BridgeAR commented Feb 7, 2018

BridgeAR left a comment

BridgeAR commented Feb 7, 2018 •

edited

nodejs / node Public

tools: auto fix custom eslint rule for prefer-assert-iferror.js #16648

tools: auto fix custom eslint rule for prefer-assert-iferror.js #16648

Conversation

shobhitchittora commented Oct 31, 2017 • edited

Checklist

Affected core subsystem(s)

apapirovski left a comment • edited

shobhitchittora commented Oct 31, 2017

shobhitchittora commented Nov 1, 2017

apapirovski commented Nov 1, 2017 • edited

shobhitchittora commented Nov 1, 2017 • edited

apapirovski Nov 1, 2017 • edited

Choose a reason for hiding this comment

shobhitchittora Nov 1, 2017

Choose a reason for hiding this comment

apapirovski Nov 1, 2017

Choose a reason for hiding this comment

shobhitchittora Nov 1, 2017

Choose a reason for hiding this comment

apapirovski left a comment

gibfahn left a comment

apapirovski commented Nov 1, 2017

shobhitchittora commented Nov 2, 2017

apapirovski commented Nov 2, 2017

not-an-aardvark commented Nov 4, 2017

joyeecheung commented Nov 6, 2017

BridgeAR commented Nov 23, 2017

apapirovski commented Nov 23, 2017

not-an-aardvark commented Nov 23, 2017

BridgeAR commented Nov 23, 2017

BridgeAR commented Dec 6, 2017

shobhitchittora commented Dec 10, 2017

apapirovski Dec 10, 2017 • edited

Choose a reason for hiding this comment

BridgeAR Jan 5, 2018

Choose a reason for hiding this comment

shobhitchittora Jan 5, 2018

Choose a reason for hiding this comment

BridgeAR Jan 19, 2018

Choose a reason for hiding this comment

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 19, 2018

BridgeAR commented Jan 31, 2018 • edited

shobhitchittora commented Jan 31, 2018

shobhitchittora commented Feb 4, 2018

BridgeAR commented Feb 7, 2018

BridgeAR left a comment

BridgeAR commented Feb 7, 2018 • edited

shobhitchittora commented Oct 31, 2017 •

edited

apapirovski left a comment •

edited

apapirovski commented Nov 1, 2017 •

edited

shobhitchittora commented Nov 1, 2017 •

edited

apapirovski Nov 1, 2017 •

edited

apapirovski Dec 10, 2017 •

edited

BridgeAR commented Jan 31, 2018 •

edited

BridgeAR commented Feb 7, 2018 •

edited