New: Autofix some one-var reports #9671

j-f1 · 2017-12-01T11:30:15Z

What is the purpose of this pull request?

Add autofixing to a rule

Addresses part of, but does not close #9072

What changes did you make? (Give an overview)

I added autofixing to the “Split [...] declarations into multiple statements” messages. It replaces each , in a variable declaration with ; var (with var being replaced with let or const as appropriate)

Is there anything you'd like reviewers to focus on?

Should I keep the last commit, or would it be better to remove it from this PR?

…#9072)

…-var

not-an-aardvark

Thanks for the pull request! This generally looks good. I have a few suggestions for tests.

not-an-aardvark · 2017-12-05T21:17:53Z

tests/lib/rules/one-var.js

@@ -242,6 +243,9 @@ ruleTester.run("one-var", rule, {
        },
        {
            code: "function foo() { var bar = true; var baz = false; }",
+
+            // output: "function foo() { var bar = true, baz = false; }",


Can you remove the commented code here?

not-an-aardvark · 2017-12-05T21:20:01Z

lib/rules/one-var.js

+            if (!filter) {
+                filter = () => true;
+            }
+            return function *fix(fixer) {


In #8101 (comment) it seemed like a few people didn't want to use generator functions in the codebase, although I'm personally fine with it.

Another option would be to make fix return an array, and use .map rather than a for-of loop.

👍 I’ll use arrow functions!

not-an-aardvark · 2017-12-05T21:22:45Z

lib/rules/one-var.js

+         * @returns {boolean}       If the node has a `;` after it
+         */
+        function hasSemi(node) {
+            return sourceCode.getLastToken(node).type === "semi";


I'm confused by this line because semicolon tokens have type Punctuator, not semi (e.g. see the tokens list in this AST). Are you sure this function ever returns true?

Turns out this is dead code, so I’m going to 🔥 it.

not-an-aardvark · 2017-12-05T21:23:56Z

lib/rules/one-var.js

+         * @param   {...VariableDeclaration} declarations The `VariableDeclarations` to merge
+         * @returns {Function}                            The fixer function
+         */
+        function mergeDeclarations() { // eslint-disable-line no-unused-vars


I think it would be better to remove the unused code for now, and then add it afterwards once it's complete.

not-an-aardvark · 2017-12-05T21:26:23Z

tests/lib/rules/one-var.js

@@ -581,6 +646,7 @@ ruleTester.run("one-var", rule, {
        },
        {
            code: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3, baz = z; for (var d in baz) {}}",
+            output: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3; var baz = z; for (var d in baz) {}}",


Can you add some tests that use destructuring, and different types of declarations?

For example:

var {foo} = 1, [bar] = 2; // fixed to var {foo} = 1; var [bar] = 2;

const foo = 1, bar = 2; // fixed to const foo = 1; const bar = 2;

not-an-aardvark · 2017-12-05T21:28:52Z

tests/lib/rules/one-var.js

@@ -581,6 +646,7 @@ ruleTester.run("one-var", rule, {
        },
        {
            code: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3, baz = z; for (var d in baz) {}}",
+            output: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3; var baz = z; for (var d in baz) {}}",


Can you add a test where a variable declaration spans multiple lines?

For example, I would expect the following code

var foo = 1, bar = 2;

...to get fixed to something like this:

var foo = 1; var bar = 2;

It would also be fine for it to get fixed to something like this, because the indent rule would adjust it afterwards:

var foo = 1; var bar = 2;

Here’s what I have now:

var foo = 1; var bar = 2;

I think if possible, this should be changed to the output that @not-an-aardvark suggested. We have no other rule that would be able to fix extra line break for the second declaration.

not-an-aardvark · 2017-12-05T21:37:47Z

tests/lib/rules/one-var.js

@@ -581,6 +646,7 @@ ruleTester.run("one-var", rule, {
        },
        {
            code: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3, baz = z; for (var d in baz) {}}",
+            output: "var a = 1; var b = 2; var x, y; for (var z of foo) {var c = 3; var baz = z; for (var d in baz) {}}",


Can you add a test where there is a comment between the variable declarators?

var foo = 1, // comment bar = 2;

I would expect the comment to be preserved and appear somewhere in the output.

Here’s what I have now:

var foo = 1; var // comment bar = 2;

Do you think it would be possible to change that? I think putting var and bar on separate lines would be surprising to users.

… for one-var" This reverts commit 20b8d3d.

(Also make them pass)

ilyavolodin

Could you remove package-lock.json please?

j-f1 · 2017-12-08T21:37:21Z

Could you remove package-lock.json please?

👍 ✅

kaicataldo · 2018-01-24T22:49:36Z

Sorry, looks like we lost track of this :( Aside from the merge conflicts, where do we stand with this?

not-an-aardvark · 2018-01-24T22:52:25Z

It would be good to change #9671 (comment) -- aside from that I think this is ready to merge.

j-f1 · 2018-01-24T22:53:56Z

There’s also #9671 (comment). I haven’t had much time to work on this, and those issues seem pretty hard to fix, so I haven’t fixed them :(

kaicataldo · 2018-01-24T22:56:50Z

Thanks for the quick follow-up!

@j-f1 Do you plan on finishing this at some point? Is there anything you need from us?

j-f1 · 2018-02-08T23:42:36Z

I haven’t gotten a chance to address the comments, and I don’t have enough experience with fixers to do this properly. Closing.

Nokel81 · 2018-03-01T19:27:30Z

I am looking into continuing this work. I will open a new PR when I have worked through #9671 (comment)

j-f1 added 3 commits November 27, 2017 17:10

Chore: Add failing tests for one-var autofixing

6f2cc66

New: Fix the “Split [...] declarations” errors in one-var (ref eslint…

e3a0043

…#9072)

Chore: Add some future autofixing goals and a helper function for one…

20b8d3d

…-var

j-f1 changed the title ~~Autofix some one-var rules~~ New: Autofix some one-var rules Dec 1, 2017

aladdin-add added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Dec 1, 2017

Replace .start with .range[0] and .end with .range[1]

078cf7f

not-an-aardvark suggested changes Dec 5, 2017

View reviewed changes

j-f1 added 4 commits December 5, 2017 17:19

Revert "Chore: Add some future autofixing goals and a helper function…

e000302

… for one-var" This reverts commit 20b8d3d.

Remove unused and broken function

6d21169

De-generatorify the fixer

00f099d

Add recommended tests from code review

a3d0357

(Also make them pass)

ilyavolodin suggested changes Dec 8, 2017

View reviewed changes

🔥 package-lock.json

e558e33

j-f1 changed the title ~~New: Autofix some one-var rules~~ New: Autofix some one-var messages Dec 12, 2017

j-f1 changed the title ~~New: Autofix some one-var messages~~ New: Autofix some one-var reports Dec 12, 2017

aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 13, 2017

j-f1 closed this Feb 8, 2018

eslint-deprecated bot locked and limited conversation to collaborators Aug 8, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Autofix some one-var reports #9671

New: Autofix some one-var reports #9671

j-f1 commented Dec 1, 2017

not-an-aardvark left a comment

not-an-aardvark Dec 5, 2017

not-an-aardvark Dec 5, 2017

j-f1 Dec 6, 2017

not-an-aardvark Dec 5, 2017

j-f1 Dec 6, 2017

not-an-aardvark Dec 5, 2017

not-an-aardvark Dec 5, 2017

j-f1 Dec 6, 2017

not-an-aardvark Dec 5, 2017

j-f1 Dec 6, 2017

ilyavolodin Dec 8, 2017

not-an-aardvark Dec 5, 2017

j-f1 Dec 6, 2017

not-an-aardvark Dec 12, 2017

ilyavolodin left a comment

j-f1 commented Dec 8, 2017

kaicataldo commented Jan 24, 2018

not-an-aardvark commented Jan 24, 2018

j-f1 commented Jan 24, 2018

kaicataldo commented Jan 24, 2018

j-f1 commented Feb 8, 2018

Nokel81 commented Mar 1, 2018 •

edited

New: Autofix some one-var reports #9671

New: Autofix some one-var reports #9671

Conversation

j-f1 commented Dec 1, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyavolodin left a comment

Choose a reason for hiding this comment

j-f1 commented Dec 8, 2017

kaicataldo commented Jan 24, 2018

not-an-aardvark commented Jan 24, 2018

j-f1 commented Jan 24, 2018

kaicataldo commented Jan 24, 2018

j-f1 commented Feb 8, 2018

Nokel81 commented Mar 1, 2018 • edited

Nokel81 commented Mar 1, 2018 •

edited