Update: Add autofix for `sort-vars` #9496

trevinhofmann · 2017-10-21T19:22:08Z

What is the purpose of this pull request?

Add autofixing to a rule

What changes did you make?

The sort-vars rule can now automatically fix unsorted variable declarations when each variable is either unassigned or assigned to a literal value. I added the implementation for this fixer, test cases to validate that the fixer works as expected, and test cases to validate that it does not fix when a variable is assigned to a non-literal value.

In short, the fixer will convert var c, b, a to var a, b, c and var b = 3, a = true to var a = true, b = 3 but will not fix var b = f(), a = g() or var b = 5, a = b.

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

All of it, please 😕

eslintbot · 2017-10-21T19:22:09Z

LGTM

aladdin-add

thanks for contribution! just left a few comments.

aladdin-add · 2017-10-22T10:11:25Z

tests/lib/rules/sort-vars.js

+        {
+            code: "var c, d, a, b",
+            output: "var a, b, c, d",
+            errors: 2


is this intentional?

Yes, this test case was already included; I only expanded it and added the expected fixed output.

It is expected to have two errors per the documentation:

Alphabetical list is maintained starting from the first variable and excluding any that are considered problems. So the following code will produce two problems:
var c, d, a, b;
But this one, will only produce one:
var c, d, a, e;

Do you mean this: errors: [expectedError, expectedError]
~~I prefer reporting one error. see here:https://github.com/eslint/eslint/blob/master/docs/developer-guide/working-with-rules.md#applying-fixes~~

Best practices for fixes:

Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

Make fixes as small as possible. Fixes that are unnecessarily large could conflict with other fixes, and prevent them from being applied.

Only make one fix per message. This is enforced because you must return the result of the fixer operation from fix().

Since all rules are run again after the initial round of fixes is applied, it's not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule.

For example, suppose a fixer would like to surround an object key with quotes, but it's not sure whether the user would prefer single or double quotes.

I'm a little confused. Are you suggesting that the rule's functionality be changed to only report one error?

I guess reporting one error can simplify the code to 3 steps:
check is sorted => check is safe to fix => do fix.
maybe just my personal preference, I don't feel strong.

hmm. sorry! I forgot this is the original functionality. not a good idea!~ 👎

aladdin-add · 2017-10-22T10:31:26Z

tests/lib/rules/sort-vars.js

+        {
+            code: "var b=10, a=b;",
+            output: null,
+            errors: [expectedError]


could you please add some more tests:
e.g.

var b = 0, a = `${b}`; var b = 0, a = `${f()}` var b = 0, c = b, a; var b = 0, c = 0, a = b + c; var b = f(), c, d, a; var b = `${f()}`, c, d, a;

aladdin-add · 2017-10-22T10:36:52Z

lib/rules/sort-vars.js

-                        currenVariableName = currenVariableName.toLowerCase();
-                    }
+                    if (currentVariableName < lastVariableName) {
+                        if (unfixable || fixed) {


Nitpick: maybe it make more sense to place this check to fixer function. (returning null means do not fix.)

Thanks! I did not know about returning null in a fixer. I will change this.

eslintbot · 2017-10-23T03:45:29Z

LGTM

platinumazure · 2017-10-23T13:30:46Z

I would love to see the reporting logic be simplified to reporting one error per VariableDeclaration-- but eventually, and not in this PR. 😄

trevinhofmann · 2017-10-23T13:37:14Z

Agreed @platinumazure and @aladdin-add - I would prefer simplified logic that reports only one error, but I did not want to change that functionality in this PR. 😃

aladdin-add

LGTM, thanks!

ilyavolodin · 2017-11-01T21:38:33Z

@eslint/eslint-team Does anyone else wants to support this enhancement?

trevinhofmann · 2017-11-22T15:36:41Z

If this does get merged, I would gladly submit a second PR to simplify the logic a bit and report only one error.

platinumazure · 2017-11-22T15:59:30Z

@eslint/eslint-team This just needs a champion and then we can accept. Otherwise, we should probably close this soon (though I really hope we don't).

platinumazure

I think I'm okay with this approach, but I don't know if we have precedent with having multiple lint messages return a fixer that's conditional on whether a closure variable has been set or not. But then again, it's really hard conceptually to have a "simple" design around a rule that needs to basically report individual VariableDeclarator nodes for users when autofix isn't on, and report/fix the VariableDeclaration when autofix is on. So there probably isn't a better way to do this given rules can't know if autofix is on or not. But this might be worth revisiting/refactoring later.

not-an-aardvark · 2017-11-26T02:51:37Z

Thanks for contributing!

Update: Add autofix for sort-vars

33396d5

aladdin-add requested changes Oct 22, 2017

View reviewed changes

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 Oct 22, 2017

Chore: Add test cases and small refactor for sort-vars

2ab9c37

aladdin-add approved these changes Oct 28, 2017

View reviewed changes

ilyavolodin approved these changes Nov 1, 2017

View reviewed changes

aladdin-add self-assigned this Nov 23, 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 Nov 23, 2017

platinumazure approved these changes Nov 26, 2017

View reviewed changes

not-an-aardvark approved these changes Nov 26, 2017

View reviewed changes

not-an-aardvark merged commit 5619910 into eslint:master Nov 26, 2017

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

eslint-deprecated bot locked and limited conversation to collaborators May 26, 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 May 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Add autofix for `sort-vars` #9496

Update: Add autofix for `sort-vars` #9496

trevinhofmann commented Oct 21, 2017 •

edited

eslintbot commented Oct 21, 2017

aladdin-add left a comment

aladdin-add Oct 22, 2017

trevinhofmann Oct 23, 2017

aladdin-add Oct 23, 2017 •

edited

trevinhofmann Oct 23, 2017

aladdin-add Oct 23, 2017 •

edited

aladdin-add Oct 22, 2017 •

edited

aladdin-add Oct 22, 2017

trevinhofmann Oct 23, 2017

eslintbot commented Oct 23, 2017

platinumazure commented Oct 23, 2017

trevinhofmann commented Oct 23, 2017

aladdin-add left a comment

ilyavolodin commented Nov 1, 2017

trevinhofmann commented Nov 22, 2017

platinumazure commented Nov 22, 2017

platinumazure left a comment

not-an-aardvark commented Nov 26, 2017

Update: Add autofix for sort-vars #9496

Update: Add autofix for sort-vars #9496

Conversation

trevinhofmann commented Oct 21, 2017 • edited

eslintbot commented Oct 21, 2017

aladdin-add left a comment

Choose a reason for hiding this comment

aladdin-add Oct 22, 2017

Choose a reason for hiding this comment

trevinhofmann Oct 23, 2017

Choose a reason for hiding this comment

aladdin-add Oct 23, 2017 • edited

Choose a reason for hiding this comment

trevinhofmann Oct 23, 2017

Choose a reason for hiding this comment

aladdin-add Oct 23, 2017 • edited

Choose a reason for hiding this comment

aladdin-add Oct 22, 2017 • edited

Choose a reason for hiding this comment

aladdin-add Oct 22, 2017

Choose a reason for hiding this comment

trevinhofmann Oct 23, 2017

Choose a reason for hiding this comment

eslintbot commented Oct 23, 2017

platinumazure commented Oct 23, 2017

trevinhofmann commented Oct 23, 2017

aladdin-add left a comment

Choose a reason for hiding this comment

ilyavolodin commented Nov 1, 2017

trevinhofmann commented Nov 22, 2017

platinumazure commented Nov 22, 2017

platinumazure left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Nov 26, 2017

Update: Add autofix for `sort-vars` #9496

Update: Add autofix for `sort-vars` #9496

trevinhofmann commented Oct 21, 2017 •

edited

aladdin-add Oct 23, 2017 •

edited

aladdin-add Oct 23, 2017 •

edited

aladdin-add Oct 22, 2017 •

edited