color.js icon indicating copy to clipboard operation
color.js copied to clipboard

Eslint is not being run on CI... and lack of indent rule produces odd fixes

Open Artoria2e5 opened this issue 3 years ago • 3 comments

I was trying to use eslint to fix my code style once and for all -- so I don't need to manually change them. But it first turns out that we don't really have a test set up to run it, not even in package.json. So I manually ran node_modules/.bin/eslint src, and sure enough the entire repo has been a bit sloppy too:

.....\color.js\src\deltaE\deltaEITP.js
  24:2  warning  Missing semicolon  semi

.....\GH\color.js\src\deltaE\deltaEJz.js
  32:2  warning  Closing curly brace appears on the same line as the subsequent
block  brace-style
  36:2  warning  Closing curly brace appears on the same line as the subsequent
block  brace-style

I figured re-running with --fix would solve the issue, but check out what I got instead:

diff --git a/src/deltaE/deltaEJz.js b/src/deltaE/deltaEJz.js
index 80f116e..5ad170b 100644
--- a/src/deltaE/deltaEJz.js
+++ b/src/deltaE/deltaEJz.js
@@ -29,11 +29,13 @@ Color.prototype.deltaEJz = function (sample) {
                // both undefined hues
                Hz1 = 0;
                Hz2 = 0;
-       } else
+       }
+ else
        if (Number.isNaN(Hz1)) {
                // one undefined, set to the defined hue
                Hz1 = Hz2;
-       } else
+       }
+ else
        if (Number.isNaN(Hz2)) {
                Hz2 = Hz1;
        }

Yep, it broke the indentation. I think.

(I don't exactly think the original putting else and if on different lines is normal either, but a more conventional else if got put in an unindented line too.)

Artoria2e5 avatar Jun 27 '21 14:06 Artoria2e5

Yep, adding an indent setting seems to fix the fix. I will do some more tinkering to check out some other rules before I do the PR.

Artoria2e5 avatar Jun 27 '21 14:06 Artoria2e5

The recommended rules does pick out some fishy things, and the false-positive rate does get reduced with appropriate env settings... But I still don't think it's my call to make. Heck, the indent thing alone will create a big blame-disturbing commit if fixed.

Artoria2e5 avatar Jun 27 '21 14:06 Artoria2e5

Yeah, I think we both tend to use it as an editor plugin, rather than a terminal command, that's why there was nothing there.

Do avoid auto fix for block-level things, as it will screw up the indentation as you noticed. We use smart tabs, that unfortunately many tools do poorly with.

@svgeesus these are files you edited, could you take a look please?

LeaVerou avatar Jun 29 '21 08:06 LeaVerou