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:06Z

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:16Z

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 <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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

Landed in d797775 🎉

PR-URL: #16648 Refs: #16636 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#16648 Refs: nodejs#16636 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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

apapirovski reviewed Oct 31, 2017

View reviewed changes

apapirovski reviewed Nov 1, 2017

View reviewed changes

apapirovski approved these changes Nov 1, 2017

View reviewed changes

gibfahn approved these changes Nov 1, 2017

View reviewed changes

jasnell approved these changes Nov 2, 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

apapirovski approved these changes Dec 10, 2017

View reviewed changes

apapirovski reviewed Dec 10, 2017

View reviewed changes

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 10, 2017

BridgeAR approved these changes Dec 10, 2017

View reviewed 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 December 11, 2017 05:01

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 January 26, 2018 22:02

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

shobhitchittora added 2 commits February 4, 2018 09:39

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2018

BridgeAR approved these changes Feb 7, 2018

View reviewed 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

apapirovski left a comment •

edited

Loading

shobhitchittora commented Oct 31, 2017

shobhitchittora commented Nov 1, 2017

apapirovski commented Nov 1, 2017 •

edited

Loading

shobhitchittora commented Nov 1, 2017 •

edited

Loading

apapirovski Nov 1, 2017 •

edited

Loading

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

Loading

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

Loading

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

Loading

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 Loading

Checklist

Affected core subsystem(s)

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

shobhitchittora commented Oct 31, 2017

shobhitchittora commented Nov 1, 2017

apapirovski commented Nov 1, 2017 • edited Loading

shobhitchittora commented Nov 1, 2017 • edited Loading

apapirovski Nov 1, 2017 • edited Loading

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

Choose a reason for hiding this comment

gibfahn left a comment

Choose a reason for hiding this 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 Loading

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 Loading

shobhitchittora commented Jan 31, 2018

shobhitchittora commented Feb 4, 2018

BridgeAR commented Feb 7, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 7, 2018 • edited Loading

shobhitchittora commented Oct 31, 2017 •

edited

Loading

apapirovski left a comment •

edited

Loading

apapirovski commented Nov 1, 2017 •

edited

Loading

shobhitchittora commented Nov 1, 2017 •

edited

Loading

apapirovski Nov 1, 2017 •

edited

Loading

apapirovski Dec 10, 2017 •

edited

Loading

BridgeAR commented Jan 31, 2018 •

edited

Loading

BridgeAR commented Feb 7, 2018 •

edited

Loading